Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/632: support stream-channels query parameter when adding #633

Merged
merged 5 commits into from Jan 8, 2019

Conversation

hsanjuan
Copy link
Collaborator

@hsanjuan hsanjuan commented Jan 4, 2019

Passing ?stream-channels=true will keep the current behaviour. The AddedOutput objects are streamed to the client as they happen. stream-channels=false will, however, buffer all the addedOutput objects and return them as a final json array. Stream-related headers are adjusted accordingly.

The stream-channels is used because that's what ipfs uses (even though go-ipfs does not respect it).

The proxy /add will also acquire this feature.

Additionally, this modifies ipfs-cluster-ctl add -Q flag to apply as well when --enc=json is provided. This means that, in this case, only the last json-formatted AddedOutput object will be printed (before we ignored this flag in this case and printed everything). This should facilitate parsing the root hash result when using ipfs-cluster-ctl by having valid json returned.

(This PR does not add client support for stream-channels=false. Might do that in a different PR if necessary for #632 , but I suspect it might be enough with the -Q flag change).

License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
This will only print the latest JSON update when adding
with -Q.

License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
@hsanjuan hsanjuan self-assigned this Jan 4, 2019
@ghost ghost added the status/in-progress In progress label Jan 4, 2019
@coveralls
Copy link

coveralls commented Jan 4, 2019

Coverage Status

Coverage decreased (-0.009%) to 65.459% when pulling c51ff00 on feat/632-stream-channels into b44c7fb on master.

Copy link
Contributor

@kishansagathiya kishansagathiya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments,
Will review again once this PR is complete

adder/adderutils/adderutils.go Outdated Show resolved Hide resolved
adder/adderutils/adderutils.go Outdated Show resolved Hide resolved
fmt.Println(last.Cid)
} else {
formatResponse(c, *last, nil)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code that makes use of StreamChannels bool flag is yet to be added, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's set to true by the DefaultParams() function. We won't add supports for stream-channels=false here for the moment (I think it might be enough for the requester with handling -Q better).

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

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

@kishansagathiya kishansagathiya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK Tested to be working well locally. Jenkins doesn't seem to be failing due to normal flaky tests, so let's fix that first.

@hsanjuan
Copy link
Collaborator Author

hsanjuan commented Jan 8, 2019

@lanzafame actually when NOT passing &stream-channels this will use the default of true. I did not realize that our query parser is so clever. So no need to change anything on that regard.

@hsanjuan
Copy link
Collaborator Author

hsanjuan commented Jan 8, 2019

@kishansagathiya seems the macos builder is not working well. will try another rebuild.

@hsanjuan
Copy link
Collaborator Author

hsanjuan commented Jan 8, 2019

@kishansagathiya Looks good to you?

@hsanjuan hsanjuan merged commit 595db7a into master Jan 8, 2019
@ghost ghost removed the status/in-progress In progress label Jan 8, 2019
@hsanjuan hsanjuan deleted the feat/632-stream-channels branch January 8, 2019 12:31
@hsanjuan hsanjuan mentioned this pull request Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants