Support default in addition to defaults #88

slycoder · 2018-04-19T06:42:34Z

initialize should write to the default_section.
handle the case where initialize is called without a section.
handle legacy config files with default instead of defaults.
handle config files without a profile section aside from the defaults.

* initialize should write to the default_section. * handle the case where initialize is called without a section. * handle legacy config files with default instead of defaults. * handle config files without a profile section aside from the defaults.

drewsonne

If I understand you correctly...

This PR would treat the defaults section as if it were a fully configure section in the config file and if there were no other sections in the file, and the user had not specified one, we would use the defaults section. Is that right?

drewsonne · 2018-04-19T06:43:13Z

onelogin_aws_cli/configuration.py

 def __init__(self, config_file=None):
 super().__init__(default_section='defaults')

- self['defaults'] = dict(save_password=False)
+ self[self.default_section] = self.DEFAULTS


Good stuff.

drewsonne · 2018-04-19T06:46:26Z

onelogin_aws_cli/configuration.py

+ # For backwards compatibility, we check if they have a default
+ # section instead of a defaults section and do a little switcheroo as
+ # needed.
+ if self.has_section('default') and not self.has_defaults:


This might get a bit confusing.
I saw:

the default section as being the config which is used when no arguments are provided to the cli.

the defaults section as being an "abstract" config which will fill in the gaps in the other configs.

If we say defaults and default are the same, which config is picked when no cli args are set? I would like to be be able to set some sort of default config, as for the most time I would be setting the same option again and again on the cli (onelogin-aws-login -C my_config)

drewsonne · 2018-04-19T06:49:13Z

onelogin_aws_cli/configuration.py


 self.file = config_file

 if self.file is not None:
 self.load()

+ @property
+ def has_defaults(self) -> bool:
+ """True if the defaults section has settings beyond our defaults"""


I... am confused...? what's the difference between the defaults section and the defaults?

I can clarify the comment, but I just meant that if it's properly initialized, they should have something beyond save_password (i.e., "our defaults") which we inject into the defaults section.

"""True if the defaults section in the config file has key/values in addition to those defined in onelogin_aws_cli.ConfigurationFile.DEFAULTS"""

am I understanding that right?

Yep, that's much more eloquent (although it occurs to me I should make the check more accurate than it is now).

codecov-io · 2018-04-19T07:05:52Z

Codecov Report

Merging #88 into master will increase coverage by 0.25%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #88      +/-   ##
==========================================
+ Coverage   74.02%   74.27%   +0.25%     
==========================================
  Files           6        6              
  Lines         308      311       +3     
==========================================
+ Hits          228      231       +3     
  Misses         80       80

Impacted Files	Coverage Δ
onelogin_aws_cli/configuration.py	`93.75% <100%> (+0.3%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3258bc3...3a3d7f2. Read the comment docs.

drewsonne · 2018-04-19T07:06:48Z

Looping in @mumoshu for their interest

slycoder · 2018-04-19T07:13:32Z

I guess at a top-level we need to decide how defaults + profiles should work. It just seemed to me odd that the config file wouldn't work with only a defaults section.

In allowing that, this PR changed a bunch of places where it assumed the existence of a non-"defaults" profile, while at the same time supporting a "default"-only legacy config file.

@drewsonne I think your reasoning makes sense for how you'd want to deploy the configs in a lot of environments; I'm just uncertain as to whether that should be "enforced".

drewsonne · 2018-04-19T07:21:44Z

I'm with you so far, and not interested in enforcing any particular workflow, but I am selfishly hoping my use case will work. :-)

Can I put some points out and we'll see where we agree/disagree?

There is a defaults (with an s) section, which is used as both a configuration profile unto itself and an "abstract" configuration profile which other configuration profiles will automatically pull their values from if they are missing any.
If 1. is implemented, there is no need for a "special" default (without an s) section, as its functionality would be covered by the defaults (with an s) section.
The configuration profile which should be chosen if the user does not provide one is defaults.
As an example,

[defaults]
base_uri = https://api.us.onelogin.com/
subdomain = mycompany
username = john@mycompany.com
save_password = yes
client_id = f99ee51f00400649280db1028ffa3ca9b21b680f2189b238d342cc8158c401c7
client_secret = a85234b6db01a29a493e2422d7930dffe6f4d3a826270a18838574f6b8ef7c3e
aws_app_id = 555027 # r&d

[testing]
aws_app_id = 555029 # testing

[live]
aws_app_id = 555070 # live

If I wanted to log into my "r&d" account, I will call:

$ onelogin-aws-login

or

$ onelogin-aws-login -C defaults

testing account:

$ onlogin-aws-login -C testing

staging account:

$ onelogin-aws-login -C staging

Is that right/reasonable?

slycoder · 2018-04-19T07:32:40Z

Agree on both counts =).

drewsonne · 2018-04-19T07:34:32Z

I made an edit, and shan't edit more. All 4 points are ok?

slycoder · 2018-04-19T07:40:16Z

Agree on everything.

slycoder · 2018-04-19T19:01:14Z

onelogin_aws_cli/configuration.py

@@ -68,7 +85,10 @@ def section(self, section_name: str):
 :param section_name: Name of the section in the config file
 :return:
 """
- if not self.has_section(section_name):
+ if not section_name:


@drewsonne This is the check I was referring to in regards to default section handling

* The config value should always fall back to `default` * Specify defaults section as discussed in #88 * Add more verbose error handling * Don’t include empty values in overrides * Moved defaults from cli into config file * Fix flake8 * Fix test cases * Handle defaults so they are not written to the config * Updated test cases to handle config precedence * Removed defaults check * Make sure duration is an int

slycoder · 2018-04-21T22:14:45Z

Alright, this PR now up-to-date. @drewsonne , do you think this PR is still worthwhile.

drewsonne · 2018-04-22T06:34:38Z

onelogin_aws_cli/configuration.py


 def initialise(self, config_name='defaults'):
 """
 Prompt the user for configurations, and save them to the
 onelogin-aws-cli config file
 """
 print("Configure Onelogin and AWS\n\n")
+ if not config_name:


This will never be falsey unless calling functions pass a None value in. If that's the case, the default value for the method param should probably just be config_name = None.

Oops, this got leftover from the merge.

drewsonne · 2018-04-22T06:37:25Z

onelogin_aws_cli/configuration.py

+ if self.has_section('default') and not self.has_defaults:
+ print("It looks like you're using a the deprecated 'default' "
+ "section.\nConsider renaming the section to 'defaults'.")
+ self.default_section = 'default'


In regards to whether this is still relevant, it depends on how we want to use default. I will write some bits in the config section of the README and we can regroup and see if it is needed. Sound ok?

Sure this was just in response to the reported issue; upon investigating I realized that until recently initialization was writing to a profile named 'default' so it seems like we should support it (or do a major version bump).

Ah right. Your call. At the very least, I'll write details for the configuration behaviour. If you do close it, we can always come back to this branch.

Sounds good, thanks. I'll pull the default swizzling into a separate PR and we can discuss there.

slycoder added 3 commits April 18, 2018 23:42

Better message when there's no config

ff98348

Be a bit more careful about guarding defaults

1190f83

drewsonne reviewed Apr 19, 2018

View reviewed changes

Simplify the test

3fbefc1

drewsonne added a commit to drewsonne/onelogin-aws-cli that referenced this pull request Apr 19, 2018

Specify defaults section as discussed in physera#88

3a382e6

slycoder commented Apr 19, 2018

View reviewed changes

drewsonne added a commit to drewsonne/onelogin-aws-cli that referenced this pull request Apr 21, 2018

Specify defaults section as discussed in physera#88

afdc313

drewsonne added a commit to drewsonne/onelogin-aws-cli that referenced this pull request Apr 21, 2018

Specify defaults section as discussed in physera#88

68b1733

Merge branch 'master' into better-handling-of-defaults

37ea1f7

slycoder changed the title ~~Fix a number of bugs relating to config file defaults~~ Support default in addition to defaults Apr 21, 2018

slycoder added 2 commits April 21, 2018 13:36

Revert a change

b8c01b6

Fix tests

39db094

drewsonne reviewed Apr 22, 2018

View reviewed changes

slycoder added 3 commits April 21, 2018 23:41

Remove errant lines

10826cd

Merge branch 'master' into better-handling-of-defaults

608cdc2

Merge branch 'master' into better-handling-of-defaults

3a3d7f2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support default in addition to defaults #88

Support default in addition to defaults #88

slycoder commented Apr 19, 2018

drewsonne left a comment

drewsonne Apr 19, 2018

drewsonne Apr 19, 2018

drewsonne Apr 19, 2018 •

edited

Loading

slycoder Apr 19, 2018

drewsonne Apr 19, 2018 •

edited

Loading

slycoder Apr 19, 2018

codecov-io commented Apr 19, 2018 •

edited

Loading

drewsonne commented Apr 19, 2018

slycoder commented Apr 19, 2018

drewsonne commented Apr 19, 2018 •

edited

Loading

slycoder commented Apr 19, 2018

drewsonne commented Apr 19, 2018

slycoder commented Apr 19, 2018

slycoder Apr 19, 2018

slycoder commented Apr 21, 2018

drewsonne Apr 22, 2018

slycoder Apr 22, 2018

drewsonne Apr 22, 2018

slycoder Apr 22, 2018

drewsonne Apr 22, 2018

slycoder Apr 22, 2018

Support default in addition to defaults #88

Are you sure you want to change the base?

Support default in addition to defaults #88

Conversation

slycoder commented Apr 19, 2018

drewsonne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewsonne Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewsonne Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 19, 2018 • edited Loading

Codecov Report

drewsonne commented Apr 19, 2018

slycoder commented Apr 19, 2018

drewsonne commented Apr 19, 2018 • edited Loading

slycoder commented Apr 19, 2018

drewsonne commented Apr 19, 2018

slycoder commented Apr 19, 2018

Choose a reason for hiding this comment

slycoder commented Apr 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewsonne Apr 19, 2018 •

edited

Loading

drewsonne Apr 19, 2018 •

edited

Loading

codecov-io commented Apr 19, 2018 •

edited

Loading

drewsonne commented Apr 19, 2018 •

edited

Loading