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

interface/grpc/core>deploy: minor improvements #642

Closed
wants to merge 3 commits into from

Conversation

ilgooz
Copy link
Contributor

@ilgooz ilgooz commented Dec 13, 2018

No description provided.

@ilgooz
Copy link
Contributor Author

ilgooz commented Dec 13, 2018

needs review

Copy link
Member

@NicolasMahe NicolasMahe left a comment

Choose a reason for hiding this comment

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

Nicely done 👍
Way easier to understand and to add more feature in the future.

@NicolasMahe NicolasMahe requested a review from krhubert December 14, 2018 12:03
@ilgooz ilgooz self-assigned this Dec 17, 2018
@ilgooz ilgooz added this to the v0.6 milestone Dec 17, 2018
Copy link
Contributor

@krhubert krhubert left a comment

Choose a reason for hiding this comment

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

This PR makes conflict with #641 and #641 has greater priority than this. Also, I've already taken some comments and pushed into #641. So let's focus on features first.

@antho1404
Copy link
Member

@krhubert maybe it would make more sense to do the changes on this PR and we merge this one when it's about the stream for the deploy.
Let's try to keep PR as small as possible and #641 should be only about the feature of #641

@NicolasMahe
Copy link
Member

The implementation of the reader from the gRPC stream done by @krhubert on #641 is actually simpler and doesn't involve any other parameter (eg: URL).
The good part is that the reader is only reading chunk from the stream, so if we add more parameter to the stream, the reader doesn't need to change.

https://github.com/mesg-foundation/core/blob/c8775575b921c22b09030d7f4d2c3495391dd522/interface/grpc/core/deploy.go#L97-L119x

I think this PR is not necessary anymore.

@ilgooz
Copy link
Contributor Author

ilgooz commented Dec 20, 2018

The implementation of the reader from the gRPC stream done by @krhubert on #641 is actually simpler and doesn't involve any other parameter (eg: URL).
The good part is that the reader is only reading chunk from the stream, so if we add more parameter to the stream, the reader doesn't need to change.

https://github.com/mesg-foundation/core/blob/c8775575b921c22b09030d7f4d2c3495391dd522/interface/grpc/core/deploy.go#L97-L119x

I think this PR is not necessary anymore.

I don't agree with it because putting a simple RecvMessage() func to stream reader and re-using it in the io.Reader implementation and in the deploy gRPC handler(DeployService) was my first intention while creating the stream reader and to me it's way easier to use. You can check this link to compare. Feel free to close this PR if you don't agree.

@ilgooz ilgooz added the on hold label Dec 20, 2018
@ilgooz
Copy link
Contributor Author

ilgooz commented Dec 20, 2018

this PR is now a part of a new one: #663

@ilgooz ilgooz closed this Dec 20, 2018
@ilgooz ilgooz deleted the feature/improve-deploy branch December 20, 2018 17:48
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.

4 participants