Fixes incorrect handling of cli and config file parameter merging #87
Conversation
Codecov Report
@@ Coverage Diff @@
## master #87 +/- ##
==========================================
- Coverage 73.18% 72.81% -0.38%
==========================================
Files 6 6
Lines 317 320 +3
==========================================
+ Hits 232 233 +1
- Misses 85 87 +2
Continue to review full report at Codecov.
|
drewsonne
changed the title
defaultdefaults
| @@ -17,7 +17,7 @@ def __init__(self): | |||
| self.add_argument( | |||
| '-C', '--config-name', | |||
| action=EnvDefault, required=False, | |||
| dest='config_name', | |||
| dest='config_name', default='defaults', | |||
There was a problem hiding this comment.
The stumbling block I ran into is that when you do it this way, and you try to retrieve the section, per configparser, the default section is not acknowledged by has_section so we'll try to add it. However, adding a section with the name of the default section throws an exception.
There was a problem hiding this comment.
We can change it so it errors or returns None or something if there's no section. It doesn't need to be that smart.
If no section exists, we can make it explicit that a section would need to be added. Would that fix it?
There was a problem hiding this comment.
That would but then it just moves a check somewhere else (see line 88 of my PR)
There was a problem hiding this comment.
Can you link to it? I can not see a line 88 in the diffs.
There was a problem hiding this comment.
Maybe moving the check is a good thing.
I propose we should only create a section when we run the --configure option. Any other time we should (gracefully) fail and say we don't have the section.
That is, only create a section when we explicitly want to create a new one. Any other time, say we don't have one.
There was a problem hiding this comment.
I think I've got this working now.
|
Ok, I think this change is good although one thing comes to mind --- does is_initialized work for defaults (since we automatically populate it with save_password we need to also check has_defaults)? |
drewsonne
force-pushed
the
feature/add-missing-default
branch
from
3a382e6
to
afdc313
Compare
drewsonne
changed the title
defaults
| @@ -34,7 +34,7 @@ def __init__(self): | |||
| ) | |||
|
|
|||
| self.add_argument( | |||
| '-d', '--duration-seconds', type=int, default=3600, | |||
| '-d', '--duration-seconds', type=int, | |||
There was a problem hiding this comment.
What are the pros/cons of setting defaults via the config as you have versus just putting defaults here?
There was a problem hiding this comment.
If (as an example), a user were to have the duration_seconds value in the config file, and not provide it via the cli, as the cli has precedence to overwrite values in the config file, when we parse the args, this default value of 3600 will always take precedence over the values in the config file.
If the config precedence chain has the cli options higher than the config file, then defaults must be set at the bottom level of the chain (in this case, the config file).
There was a problem hiding this comment.
Ah, that makes sense. In that case maybe the fallbackiest of defaults should go into overrides (so as to not be written into the config)?
drewsonne
force-pushed
the
feature/add-missing-default
branch
from
f1cb9ec
to
ba49e0c
Compare
onelogin_aws_cli/configuration.py
Outdated
|
|
||
| self.file = config_file | ||
|
|
||
| if self.file is not None: | ||
| self.load() | ||
|
|
||
| @property |
There was a problem hiding this comment.
All this jazz is no longer needed since we're never updating defaults with the fallbacks.
Not sure when this was lost. This addresses #84 .