cmds/pin: use coreapi/pin #5843

overbool · 2018-12-13T15:09:32Z

Related: #5717 (comment)
License: MIT
Signed-off-by: Overbool overbool.xu@gmail.com

magik6k

Thanks! First pass, but looks fairly good.

core/coreapi/interface/options/pin.go

core/coreapi/pin.go

core/commands/pin.go

magik6k

After those 3 should be good

core/commands/pin.go

core/coreapi/interface/options/pin.go

core/commands/pin.go

magik6k · 2019-01-07T12:36:46Z

There are some pin-related tests failing - https://ci.ipfs.team/blue/organizations/jenkins/IPFS%2Fgo-ipfs/detail/PR-5843/19/tests

magik6k

Code looks good, not sure about the failing tests

Stebalien · 2019-01-08T04:10:31Z

Unfortunately, it looks like the commands library was duck-typing structs with an Error field as an error. Fixed by renaming it to Errors.

In the past, errors/non-errors were indistinguishable without duck-typing (in some cases). We've now fixed that so the client should only interpret something as an error if:

The command returns a non-200 status.
The command returns the error in a trailing header.

However, we're still ducktyping for backwards compatibility for the moment.

Stebalien · 2019-01-08T04:29:58Z

Ah, nevermind, that wasn't the issue. The issue is that the error type can't be serialized. The type just needs to change to map[string]string.

(I've removed my commit)

overbool · 2019-01-17T08:17:43Z

@magik6k @Stebalien
I don't know much about the difference between Encoders and PostRun.
Should we replace Encoders with PostRun (like blockRmCmd) about this: https://github.com/ipfs/go-ipfs/blob/b36037a3a5ca71108d1d4673f4925f8b5b813d0c/core/commands/pin.go#L225-L230

magik6k · 2019-01-17T10:41:27Z

I don't know much about the difference between Encoders and PostRun.

The difference is that Encoders can run on the server when ?encoding parameter is specified. PostRun only runs on the client side. PostRun also allows to track state more easily and do things after the command has ran. Generally Encoders should be used instead of PostRun where possible.

overbool · 2019-01-17T12:50:51Z

@Stebalien @magik6k All tests passed. Could u help me review it again?

Stebalien · 2019-01-17T19:11:23Z

Let's wait for #5789 to avoid having to rework that. We should merge it within a few days.

Stebalien · 2019-01-21T22:29:48Z

This is unblocked but will need a rebase.

overbool · 2019-01-22T15:46:48Z

This is unblocked but will need a rebase.

It`s ok.

Stebalien

We'll need discuss the --force option a bit with the JS team (comments inline).

Stebalien · 2019-01-22T20:13:41Z

core/commands/pin.go

+			return nil, err
+		}
+
+		if err := api.Pin().Add(ctx, p, options.Pin.Recursive(recursive)); err != nil {


This should use rp, not p. If we're using IPNS, there's a chance it may have changed.

Stebalien · 2019-01-22T20:22:41Z

core/commands/pin.go

+				if err, ok := out.Error[k]; !ok {
+					fmt.Fprintf(w, "unpinned %s\n", k)
+				} else {
+					fmt.Fprintf(os.Stderr, "cannot unpin %s: %s\n", k, err)


We can't do this in an encoder as encoders are sometimes run server-side. Instead, it should be handled by a PostRun. That is, you can setup a "CLI" PostRun that prints errors to standard error and forwards everything else to the encoder.

Stebalien · 2019-01-22T21:01:27Z

core/commands/pin.go

+
+			id := enc.Encode(rp.Cid())
+			pins = append(pins, id)
+			if err := api.Pin().Rm(req.Context, rp, options.Pin.RmRecursive(recursive)); err != nil {


We need to discuss this with the JS team as well: https://github.com/ipfs/interface-ipfs-core/issues/127#issuecomment-456561650.

overbool · 2019-01-28T05:42:09Z

@Stebalien I think we can add --force option in another pr(#5717). This pr just replace corerepo/pin with coreapi/pin and will keep original feature that will bail out when it encounters an error.

(I had removed the Error map[string]string in this pr).

Stebalien · 2019-01-29T19:20:43Z

I think we can add --force option in another pr(#5717).

SGTM.

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

* Name them. Unfortunately, This makes the *actual* package names clear. I know this isn't "idiomatic go" but it's idiomatic go-ipfs. * Remove the duplicate import. License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

overbool requested a review from Kubuxu as a code owner December 13, 2018 15:09

overbool force-pushed the refactor/coreapi/pin-rm branch 3 times, most recently from a39a699 to 07b7fc6 Compare December 13, 2018 15:55

Stebalien requested a review from magik6k December 13, 2018 21:54

magik6k requested changes Dec 15, 2018

View reviewed changes

core/coreapi/interface/options/pin.go Show resolved Hide resolved

core/coreapi/pin.go Outdated Show resolved Hide resolved

core/commands/pin.go Outdated Show resolved Hide resolved

overbool force-pushed the refactor/coreapi/pin-rm branch 3 times, most recently from a41cecd to 31712ff Compare December 15, 2018 03:25

magik6k self-requested a review December 17, 2018 00:29

magik6k reviewed Dec 17, 2018

View reviewed changes

core/commands/pin.go Outdated Show resolved Hide resolved

core/coreapi/interface/options/pin.go Outdated Show resolved Hide resolved

core/commands/pin.go Outdated Show resolved Hide resolved

overbool force-pushed the refactor/coreapi/pin-rm branch 5 times, most recently from 2d50260 to 2956bd0 Compare December 21, 2018 04:41

Stebalien requested a review from magik6k January 5, 2019 21:53

magik6k reviewed Jan 7, 2019

View reviewed changes

magik6k mentioned this pull request Jan 7, 2019

cmds/pin: add force option for pin rm #5717

Closed

ghost assigned Stebalien Jan 8, 2019

ghost added the status/in-progress In progress label Jan 8, 2019

Stebalien force-pushed the refactor/coreapi/pin-rm branch from d446d58 to 2956bd0 Compare January 8, 2019 04:29

Stebalien added the need/author-input Needs input from the original author label Jan 9, 2019

magik6k added the topic/core-api Topic core-api label Jan 15, 2019

magik6k mentioned this pull request Jan 15, 2019

CoreAPI progress tracking issue #4498

Open

51 tasks

overbool force-pushed the refactor/coreapi/pin-rm branch 2 times, most recently from 53e260d to 8d4de98 Compare January 16, 2019 18:31

overbool force-pushed the refactor/coreapi/pin-rm branch from 8d4de98 to 0c22e09 Compare January 17, 2019 12:28

overbool changed the title ~~[WIP] cmds/pin: use coreapi/pin~~ cmds/pin: use coreapi/pin Jan 17, 2019

Stebalien added the status/blocked Unable to be worked further until needs are met label Jan 17, 2019

Stebalien removed the status/blocked Unable to be worked further until needs are met label Jan 21, 2019

Stebalien added needs rebase and removed need/author-input Needs input from the original author labels Jan 21, 2019

overbool force-pushed the refactor/coreapi/pin-rm branch from 0c22e09 to c7fd7b2 Compare January 22, 2019 14:12

Stebalien mentioned this pull request Jan 22, 2019

Add a --force flag to pin rm to ignore nonexistent pins and avoid bailing out #5716

Open

Stebalien requested changes Jan 22, 2019

View reviewed changes

overbool force-pushed the refactor/coreapi/pin-rm branch 2 times, most recently from b10dd5b to 03556a9 Compare January 28, 2019 05:18

magik6k self-requested a review January 28, 2019 15:49

overbool and others added 3 commits January 29, 2019 11:29

cmds/pin: use coreapi/pin

99feecf

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

cmds/pin: modify test

7a08cf9

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

cmds/pin: fix pin imports

e2ab620

* Name them. Unfortunately, This makes the *actual* package names clear. I know this isn't "idiomatic go" but it's idiomatic go-ipfs. * Remove the duplicate import. License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien force-pushed the refactor/coreapi/pin-rm branch from 03556a9 to e2ab620 Compare January 29, 2019 19:30

Stebalien approved these changes Jan 29, 2019

View reviewed changes

Stebalien merged commit f0a26c1 into ipfs:master Jan 29, 2019

ghost removed the status/in-progress In progress label Jan 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmds/pin: use coreapi/pin #5843

cmds/pin: use coreapi/pin #5843

overbool commented Dec 13, 2018 •

edited

Loading

magik6k left a comment

magik6k left a comment

magik6k commented Jan 7, 2019

magik6k left a comment

Stebalien commented Jan 8, 2019

Stebalien commented Jan 8, 2019 •

edited

Loading

overbool commented Jan 17, 2019

magik6k commented Jan 17, 2019

overbool commented Jan 17, 2019

Stebalien commented Jan 17, 2019

Stebalien commented Jan 21, 2019

overbool commented Jan 22, 2019

Stebalien left a comment

Stebalien Jan 22, 2019

Stebalien Jan 22, 2019

Stebalien Jan 22, 2019

overbool commented Jan 28, 2019 •

edited

Loading

Stebalien commented Jan 29, 2019

cmds/pin: use coreapi/pin #5843

cmds/pin: use coreapi/pin #5843

Conversation

overbool commented Dec 13, 2018 • edited Loading

magik6k left a comment

Choose a reason for hiding this comment

magik6k left a comment

Choose a reason for hiding this comment

magik6k commented Jan 7, 2019

magik6k left a comment

Choose a reason for hiding this comment

Stebalien commented Jan 8, 2019

Stebalien commented Jan 8, 2019 • edited Loading

overbool commented Jan 17, 2019

magik6k commented Jan 17, 2019

overbool commented Jan 17, 2019

Stebalien commented Jan 17, 2019

Stebalien commented Jan 21, 2019

overbool commented Jan 22, 2019

Stebalien left a comment

Choose a reason for hiding this comment

Stebalien Jan 22, 2019

Choose a reason for hiding this comment

Stebalien Jan 22, 2019

Choose a reason for hiding this comment

Stebalien Jan 22, 2019

Choose a reason for hiding this comment

overbool commented Jan 28, 2019 • edited Loading

Stebalien commented Jan 29, 2019

overbool commented Dec 13, 2018 •

edited

Loading

Stebalien commented Jan 8, 2019 •

edited

Loading

overbool commented Jan 28, 2019 •

edited

Loading