-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Disallow booleans in environment #2000
Disallow booleans in environment #2000
Conversation
:person_frowning: the test failures are not to do with the changes, as far as I can tell. |
Yeah, on top of that, forcing explicit quotes is really unwieldy. I'd rather we convert boolean env variables to strings instead. The small downside is that |
{ | ||
"type": "object", | ||
"patternProperties": { | ||
"^[a-zA-Z0-9_]+$": { |
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.
I think we should actually relax this to be any character (or maybe anything but dash). While bash may not support everything, swarm at least needs colons (http://docs.docker.com/swarm/scheduler/filter/#affinity-filter), so it might be easier to just allow most characters for now.
Oh, yuck. That's a pretty strong downside in my opinion (and reminds me - we've had the same problem with other options, e.g. |
@@ -37,7 +37,15 @@ | |||
|
|||
"environment": { |
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.
should this use the list_or_dict
schema? maybe there are differences between them?
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.
I didn't use list_or_dict
for this one as we define some extra constraints in the patternProperties
for the allowed key names. I was being restrictive cos I was worried about character compatibility for different systems.
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.
I think these constraints here are the same ones we'd want for labels
, and most likely for extra_hosts
as well (where are the two places that list_or_dict
is used).
What do you think about moving this to be the new definition of list_or_dict
?
If we do that, I think we'd want to change the format to something like non_bool_scalar
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.
I honestly can't remember now why I thought they had separate requirements and I can't find it in my notes. If you can't think of any issues with doing that, I think that's a good suggestion, I will amend. Thanks.
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.
Oh, hmm, so I guess dashes are valid in label and hostnames, so that is one issue. We could just relax this to "any character" which would make it usable for all three.
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.
I think that would be better in a separate PR as it effects other fields. I'll look at doing that after this one has gone in.
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.
cool, that sounds fair
I really don't like converting types to other types. I prefer the strict option (as it's implemented) of failing, and documenting it in the reference. If someone wants a string, they should use a string! If we suspect this is a common enough case, maybe we could allow for bools, but add a warning that it will be deprecated in the future. Give them a chance to upgrade and fix their config to use strings, and then remove it in the next release? |
+1. I do suspect it's common - there must be lots out there like this: environment:
DEBUG: true |
But in most cases a literal value (without explicit quotes) will translate to a string in YAML, which is... fine in most situations. |
@@ -44,6 +48,20 @@ def format_ports(instance): | |||
return True | |||
|
|||
|
|||
@FormatChecker.cls_checks( | |||
format="environment") |
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.
minor: this could be a single line
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.
I had been trying to do something clever previously with calling the log.warn
instead of an exception but it didn't pan out. So yep, I'll make it one line.
979cb3e
to
03c2e34
Compare
When users were putting true/false/yes/no in the environment key, the YML parser was converting them into True/False, rather than leaving them as a string. This change will force people to put them in quotes, thus ensuring that the value gets passed through as intended. Signed-off-by: Mazz Mosley <mazz@houseofmnowster.com>
We're going to warn people that allowing a boolean in the environment is being deprecated, so in a future release we can disallow it. This is to ensure boolean variables are quoted in strings to ensure they don't get mis-parsed by YML. Signed-off-by: Mazz Mosley <mazz@houseofmnowster.com>
One of the use cases is swarm requires at least : character, so going from conservative to relaxed. Signed-off-by: Mazz Mosley <mazz@houseofmnowster.com>
03c2e34
to
4b2fd76
Compare
LGTM |
…onment Disallow booleans in environment
- general_log does not work with /dev/stdout - docker/compose#2000 true needs to be in quotes Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
This change will force people to put boolean values in quotes, thus ensuring
that the value gets passed through as intended.
Fixes #1788