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

fix sync issue on status send chans and sync issue on gRPC deploy stream sends #524

Merged
merged 9 commits into from
Oct 17, 2018

Conversation

ilgooz
Copy link
Contributor

@ilgooz ilgooz commented Oct 4, 2018

No description provided.

@ilgooz ilgooz force-pushed the feature/fix-deploy-status-chan-sync branch from 72b2dab to d31f4ce Compare October 4, 2018 07:19
@ilgooz ilgooz requested a review from a team October 4, 2018 07:33
@ilgooz ilgooz added the bugfix label Oct 4, 2018
commands/service_dev.go Outdated Show resolved Hide resolved
@NicolasMahe
Copy link
Member

What was the issue that this PR fixes?

@ilgooz
Copy link
Contributor Author

ilgooz commented Oct 5, 2018

What was the issue that this PR fixes?

it solves several synchronization bugs in the code.

for the case with gRPC, if we don't have this waiter, there is a possibility of ending the stream by sending the last message (serviceID or validationErr). In that case if we cannot send all the status messages before, they will be lost.

same thing happens in some other places where status messages are send. for example for in commands package there is a possibility that execution of dev/deploy commands will be done before printing all of the status messages to os.Stdout.

commands/service_dev.go Outdated Show resolved Hide resolved
@ilgooz ilgooz self-assigned this Oct 15, 2018
@ilgooz
Copy link
Contributor Author

ilgooz commented Oct 16, 2018

good to review

@ilgooz ilgooz requested a review from a team October 16, 2018 13:47
@ilgooz ilgooz merged commit bbc5712 into dev Oct 17, 2018
@ilgooz ilgooz deleted the feature/fix-deploy-status-chan-sync branch October 17, 2018 12:58
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.

3 participants