Fix: --wait flag in ipfs-cluster-ctl and WaitFor function #363

hsanjuan · 2018-03-28T15:24:00Z

@lanzafame this was very visibly not working when I tried it. Please test a bit better next time.

lanzafame · 2018-03-29T01:31:45Z

@hsanjuan Agreed. Could you help me see where I went wrong with the following sharness tests, as they shouldn't have passed.

From sharness/t0030-ctl-pin.sh:

expecting success:
    cid=`docker exec ipfs sh -c "dd if=/dev/urandom bs=1024 count=2048 | ipfs add -q"`
    ipfs-cluster-ctl pin add --wait "$cid" | grep -q -i "PINNED" &&
    ipfs-cluster-ctl pin ls "$cid" | grep -q "$cid" &&
    ipfs-cluster-ctl status "$cid" | grep -q -i "PINNED"

expecting success:
    cid=`ipfs-cluster-ctl --enc=json pin ls | jq -r ".[] | .cid" | head -1`
    ipfs-cluster-ctl pin rm --wait "$cid" | grep -q -i "UNPINNED" &&
    !(ipfs-cluster-ctl pin ls "$cid" | grep -q "$cid") &&
    ipfs-cluster-ctl status "$cid" | grep -q -i "UNPINNED"

expecting success:
    cid=`docker exec ipfs sh -c "dd if=/dev/urandom bs=1024 count=2048 | ipfs add -q"`
    ipfs-cluster-ctl pin add --wait --wait-timeout 2s "$cid" | grep -q -i "PINNED" &&
    ipfs-cluster-ctl pin ls "$cid" | grep -q "$cid" &&
    ipfs-cluster-ctl status "$cid" | grep -q -i "PINNED"

expecting success:
    cid=`ipfs-cluster-ctl --enc=json pin ls | jq -r ".[] | .cid" | head -1`
    ipfs-cluster-ctl pin rm --wait --wait-timeout 2s "$cid" | grep -q -i "UNPINNED" &&
    !(ipfs-cluster-ctl pin ls "$cid" | grep -q "$cid") &&
    ipfs-cluster-ctl status "$cid" | grep -q -i "UNPINNED"

Also, api/rest/client/methods_test.go is failing to build because a reference to test.SlowCid which doesn't exist?

lanzafame · 2018-03-29T02:16:47Z

ipfs-cluster-ctl/main.go

+	}
+
+	if status.Cid == nil { // no status from "wait"
+		time.Sleep(1000 * time.Millisecond)


s/1000*time.Millisecond/1*time.Second

lanzafame · 2018-03-29T02:30:12Z

ipfs-cluster-ctl/main.go

+						waitAndFormatStatusResponse(
+							c,
+							c.Bool("wait"),
+							c.Bool("no-status"),


It seems weird that waitAndFormatStatusResponse would handle the --no-status flag. Maybe handleResponseFormatFlags would be a better name and then retrieve wait and no-status from *cli.Context. Or the other way, could be to handle no-status outside of waitAndFormatStatusResponse. Not sure which would be better? @hsanjuan

did the first

lanzafame · 2018-03-29T02:32:30Z

api/rest/client/methods_test.go

+	tapi.SetClient(rpcC)
+
+	testF := func(t *testing.T, c *Client) {
+		ci, _ := cid.Decode(test.SlowCid)


test.SlowCid doesn't exist. Related question, where do the test.TestCid1, etc, come from when running the tests?

lanzafame · 2018-03-29T02:32:54Z

api/rest/client/methods.go

@@ -251,12 +252,10 @@ func (sf *statusFilter) filter(ctx context.Context, fp StatusFilterParams) {
 				return
 			}

+			sf.Out <- gblPinInfo


lanzafame · 2018-03-29T02:33:38Z

api/rest/client/methods.go

-					return status, nil
-				}
+		case st, ok := <-sf.Out:
+			if ok {


Could be simplified to:

if !ok { return status, nil } status = st

hsanjuan · 2018-03-29T08:41:42Z

@hsanjuan Agreed. Could you help me see where I went wrong with the following sharness tests, as they shouldn't have passed.

The first case in a switch was case !c.Bool("no-status"). Since that flag is set to false by default, it waited 1 second and print a status (in which everything was pinned already because it's fast enough). So the output would look like you had been waiting.

While those tests verify that the flags are actually valid and usable and that we can call the command with them, they don't fully verify the behavior. I haven't come up with a straightforward way to fully verify --wait with sharness so I left those as they are. They still will catch some errors.

coveralls · 2018-03-29T09:04:25Z

Coverage increased (+0.8%) to 67.202% when pulling 63509cf on fix/wait into f8acd4f on master.

lanzafame

LGTM, just two last nitpicks sorry.

lanzafame · 2018-03-29T09:17:52Z

ipfs-cluster-ctl/main.go

+	c *cli.Context,
+	ci *cid.Cid,
+	target api.TrackerStatus,
+	timeout time.Duration,


We could access the timeout value via cli.Context?

lanzafame · 2018-03-29T09:19:01Z

ipfs-cluster-ctl/main.go

+		return
+	}
+
+	if status.Cid == nil { // no status from "wait"


Could this go before the no-status check as it is related to the wait flag?

It couldn't, because I don't need to fetch a status if no-status is set.

Does having --wait and --no-status set at the same time make sense though?

sorry nvm, I was totally not understanding that correctly

The --wait flag was being completely ignored unless --no-status was passed too, which makes no sense because then it would print the wait status. This waits when --wait and prints the status when --no-status is not passed. If we have been waiting, the status comes from that. Otherwise we request it. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

Since no-one is reading sf.Out, sending blocks and everything hangs. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

Actually test that WaitFor waits License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

lanzafame

LGTM

hsanjuan self-assigned this Mar 28, 2018

hsanjuan requested a review from lanzafame March 28, 2018 15:24

ghost added the status/in-progress In progress label Mar 28, 2018

hsanjuan mentioned this pull request Mar 28, 2018

Release 0.3.5 #330

Closed

lanzafame suggested changes Mar 29, 2018

View reviewed changes

hsanjuan requested a review from lanzafame March 29, 2018 08:41

lanzafame suggested changes Mar 29, 2018

View reviewed changes

hsanjuan added 3 commits March 29, 2018 11:23

Fix: WaitFor never returning

4da2458

Since no-one is reading sf.Out, sending blocks and everything hangs. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

Add test for WaitFor

851e78a

Actually test that WaitFor waits License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

hsanjuan force-pushed the fix/wait branch from 63509cf to 3fb019a Compare March 29, 2018 09:24

Fix: --wait flag. Address comments

4143329

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

hsanjuan force-pushed the fix/wait branch from 3fb019a to 4143329 Compare March 29, 2018 09:25

hsanjuan requested a review from lanzafame March 29, 2018 09:25

lanzafame approved these changes Mar 29, 2018

View reviewed changes

hsanjuan merged commit 9c5ca7c into master Mar 29, 2018

ghost removed the status/in-progress In progress label Mar 29, 2018

hsanjuan deleted the fix/wait branch March 29, 2018 10:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: --wait flag in ipfs-cluster-ctl and WaitFor function #363

Fix: --wait flag in ipfs-cluster-ctl and WaitFor function #363

hsanjuan commented Mar 28, 2018

lanzafame commented Mar 29, 2018

lanzafame Mar 29, 2018

lanzafame Mar 29, 2018

hsanjuan Mar 29, 2018

lanzafame Mar 29, 2018

lanzafame Mar 29, 2018

lanzafame Mar 29, 2018

hsanjuan commented Mar 29, 2018

coveralls commented Mar 29, 2018

lanzafame left a comment

lanzafame Mar 29, 2018

lanzafame Mar 29, 2018

hsanjuan Mar 29, 2018

lanzafame Mar 29, 2018

lanzafame Mar 29, 2018 •

edited

Loading

lanzafame left a comment

Fix: --wait flag in ipfs-cluster-ctl and WaitFor function #363

Fix: --wait flag in ipfs-cluster-ctl and WaitFor function #363

Conversation

hsanjuan commented Mar 28, 2018

lanzafame commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsanjuan commented Mar 29, 2018

coveralls commented Mar 29, 2018

lanzafame left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lanzafame Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

lanzafame left a comment

Choose a reason for hiding this comment

lanzafame Mar 29, 2018 •

edited

Loading