-
Notifications
You must be signed in to change notification settings - Fork 266
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 $DNS_JSON in the same way as $PROVISIONER_JSON #2255
base: master
Are you sure you want to change the base?
Conversation
23794e5 started referring to DNS_JSON without ever defining it, so apparently this ability to override DNS settings during install via /{etc,root}/crowbar/dns.json has been broken for 3 years and 3 days ...
+1 |
@aspiers you can make shell bail out on undefined variables by setting "-u". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
99% sure it's not needed, see https://github.com/aspiers/crowbar/blob/ddda97dcc8f0bdf15eab4f00d16f76188e813710/scripts/install-chef-suse.sh#L866 where it's being defined.
@aspiers I'll close, feel free to reopen if you disagree |
The bug is there, but not for the reason I describe in this PR. If you look at the commit this PR refers to, you'll see that I wrote the code you linked to, so it's somewhat embarrassing / amusing that I had forgotten about that and misunderstood the source of the bug. The problem is that So we still need to fix this, but it will be cleaner to submit a new PR. That said, I've reopened this one as a reminder that we need to submit a new PR. |
23794e5 started referring to DNS_JSON without ever defining it, so apparently this ability to override DNS settings during install via
/{etc,root}/crowbar/dns.json
has been broken for 3 years and 3 days ...Another example of why #shellsucks - undefined variables are silently used without any error or even warning.