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

Set HOSTFILTER only if TARGET_MACHINES is set. #10147

Merged
merged 1 commit into from Feb 14, 2020

Conversation

@sengi
Copy link
Contributor

sengi commented Feb 14, 2020

Fixes bug introduced in #10146 which broke all-nodes deployments.

Tested by applying the change on the Integration Jenkins and running an all-nodes and a single-node deploy. In both cases it selected the right nodes as well as the job succeeding.

I also verified that the conditional does the right thing for TARGET_MACHINES="", TARGET_MACHINES=all and TARGET_MACHINES="foo, bar".

@@ -20,7 +20,9 @@
export DEPLOY_TO="<%= @environment -%>"
export DEPLOY_TASK="$DEPLOY_TASK"
export TAG="$TAG"
export HOSTFILTER="$TARGET_MACHINES"
if [ "${TARGET_MACHINES:-all}" != "all" ]; then

This comment has been minimized.

Copy link
@issyl0

issyl0 Feb 14, 2020

Member

if the default of "all" != "all"?

This comment has been minimized.

Copy link
@issyl0

issyl0 Feb 14, 2020

Member

I get its purpose, it just hurt my brain.

This comment has been minimized.

Copy link
@sengi

sengi Feb 14, 2020

Author Contributor

The default from the Jenkins job is "all", but we also want to handle the case where it's empty. So if it's empty, we default to "all" before doing the comparison.

This comment has been minimized.

Copy link
@sengi

sengi Feb 14, 2020

Author Contributor

Added a comment, cos I can now see this line is kinda confusing 😅 Thanks for pointing it out.

This comment has been minimized.

Copy link
@sengi

sengi Feb 14, 2020

Author Contributor

Oh wait, putting a comment there might not work without some hairy YAML escaping. Maybe I'd better not :(

@issyl0
issyl0 approved these changes Feb 14, 2020
@sengi sengi force-pushed the sengi/use-capistrano-hostfilter branch from 8ac0df6 to 457bc63 Feb 14, 2020
Fixes bug introduced in #10146 which broke all-nodes deployments.
@sengi sengi force-pushed the sengi/use-capistrano-hostfilter branch from 457bc63 to 9365972 Feb 14, 2020
@sengi sengi merged commit 43f8279 into master Feb 14, 2020
1 check passed
1 check passed
continuous-integration/jenkins/branch This commit looks good
Details
@sengi sengi deleted the sengi/use-capistrano-hostfilter branch Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.