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

Define sync channels without creating and closing them #19

Merged
merged 2 commits into from
Oct 19, 2018
Merged

Conversation

adilyse
Copy link
Contributor

@adilyse adilyse commented Oct 18, 2018

Fixes #7.

When a sync direction was disabled, the closing of a channel that
wasn't used again was causing the entire program to exit as if there
was an error.

Switched these channels to be created with var instead, which
stops this unexpected behavior.

When a sync direction was disabled, the closing of a channel that
wasn't used again was causing the entire program to exit as if there
was an error.

Switched these channels to be created with `var` instead, which
stops this unexpected behavior.
Copy link
Contributor

@mitchellh mitchellh left a comment

Choose a reason for hiding this comment

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

What you have so far is right, but I think we need to add a nil channel check around line 200/201 otherwise a interrupt (Ctrl-C) will block forever.

@adilyse
Copy link
Contributor Author

adilyse commented Oct 18, 2018

Good catch! I've added nil checks within all of the different cases in that select statement.

Copy link
Contributor

@mitchellh mitchellh left a comment

Choose a reason for hiding this comment

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

Looks good.

In general we usually unit test our commands. Its my fault that these aren't unit tested (i.e. Consul CLI is all unit tested). I'll try to add some unit tests so I can lay the groundwork since it was my mess to start with. 😄I would merge this as-is though, just noting!

@adilyse adilyse merged commit 25f084a into master Oct 19, 2018
@adilyse adilyse deleted the bugfix/7 branch October 19, 2018 23:23
@jondeandres
Copy link

hey @adilyse, thanks for this. do you have any estimation of when a new docker image will be released?

@adilyse
Copy link
Contributor Author

adilyse commented Oct 26, 2018

@jondeandres This is fix is now available in v0.2.1, just released!

@jondeandres
Copy link

hey @adilyse,thank you very much, I will try it today. Thanks for the quick fix!

wilkermichael added a commit that referenced this pull request Oct 13, 2022
wilkermichael added a commit that referenced this pull request Oct 14, 2022
wilkermichael added a commit that referenced this pull request Oct 18, 2022
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