Fix issue with passwords when passwords is provided by dict and by file #811
Conversation
|
That certainly does look like problematic code. Could you add a test that validates the fix? |
|
@Shrews I've added some tests |
|
recheck |
| @@ -150,16 +150,24 @@ def _prepare_env(self, runner_mode='pexpect'): | |||
| if self.runner_mode == 'pexpect': | |||
| try: | |||
| if self.passwords and isinstance(self.passwords, dict): | |||
| self.passwords = self.passwords.update(self.loader.load_file('env/passwords', Mapping)) | |||
| # Load password file and then the passwords dict argument for precedence | |||
There was a problem hiding this comment.
What's the reason for the change in precedence?
There was a problem hiding this comment.
Because it's useless to provide password as dictionnary if a file is already present if we don't not modify precedence.
Argument provided by "user" in python code should always be more important as config file.
There was a problem hiding this comment.
@Shrews made a good point to me
self.settings.update(self.loader.load_file('env/settings', Mapping))differs from your precedence rules for passwords here.
I agree with you sentiment
Argument provided by "user" in python code should always be more important as config file.
But that would apply to settings, just the same that it applies to passwords.
There was a problem hiding this comment.
@AlanCoding you're right.
I didn't change settings because I don't not use it and the initial fix was not intend to change settings. I just changed the obvious bug about settings.
If you insist I change change precedence with settings to, but I just need to know how to test it.
There was a problem hiding this comment.
IMO, all entries in env should have the same precedence. I feel like this PR should just fix the current bug. Changing precedence for file vs. args should likely be a separate PR and make sure that precedence is treated the same way across all possible input files.
|
Superseded by #899 |
Issue: #809
Fix the issue about
dict.update()which returnNone. It's a bad usage of this method.