Misc clarifications in deployment_utils #81
Conversation
…loyment_utils.boolean_setting helper function. Document some of the new lang functions. Add some unit tests.
Pull Request Test Coverage Report for Build 994
💛 - Coveralls |
| default=False) | ||
| help="whether this server does indexing at all", | ||
| choices=["true", "false"], | ||
| default=None) |
There was a problem hiding this comment.
Is None really what we want here? Will it pick up a default option elsewhere? Same question for --index_server, I think default should be False in that case.
There was a problem hiding this comment.
Yes, the problem is that we either have to have a rule that the command arguments override the environment variables or vice versa. The rule was (and I guess I could change this because it isn't clear it's right) that the arguments override the env variables, and so putting False here made the env variables not work. But anyway there is logic that picks up the env variables and defaults it. (This is all heavily unit tested but could still have gone by you as even then it's subtle.)
Another complexity in the design space is that env variables are strings and command arguments can be real datatypes. That's weird because when the env variables are merged, you also have to do various kinds of coercions to even get them on a level playing field, and THEN you have to flatten them again to strings to put them in an ini file and AGAIN recover them as strings later. I REALLY wish we were using a json structure to hold this data so we didn't have to do that second coerce-to-string.
There was a problem hiding this comment.
I'm leaving this alone for now.
| def test_deployment_utils_main(): | ||
|
|
||
| fake_template = "something.ini" | ||
| with override_environ(ENV_NAME='fourfront-foo'): |
There was a problem hiding this comment.
Test could use some documentation I'd say.
| return True | ||
| else: | ||
| return setting | ||
| else: # booleans, None, and odd types (though we might want to consider making other types an error). |
There was a problem hiding this comment.
Should we not coerce None into False?
There was a problem hiding this comment.
Since None is already a false value, I thought it might be better to leave it as None in case we have to debug the difference between an explicit value and no value. I could change that if you thought it wise ,but my sense was it was better to leave the thin distinction.
There was a problem hiding this comment.
Yeah I think that's a good point, so let's keep it.
| elif setting_lower == "true": | ||
| return True | ||
| else: | ||
| return setting |
There was a problem hiding this comment.
Should we not coerce whatever this could be to False?
There was a problem hiding this comment.
Again, it already functions as a false value. I really wanted to raise an error here, but was nervous about where that error would be perceived. Returning a stray value might make it easier to spot the error downstream than if we coerce it.
The main purposes of this PR are these:
Fix a bug in the defaulting of the
indexervalue in themainprogram, which was defaulting toFalse(which in turn hides the environment value ofTruethat was trying to get through). I changed the default to None from False. The logic to turn an unspecified value to False happens elsewhere already.Add an accessor
dcicutils.deployment_utils.boolean_settingwhich can be used to recover a boolean from settings that have"true"and"false"values floating around.Added unit tests.
Opportunistic:
lang_utilsfunctions and changedrelative_time_stringto allow it to take adatetime.timedeltaas an argument. (It's easier to see such needs when writing doc.)