Skip to content
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

Support default in addition to defaults #88

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
7 changes: 6 additions & 1 deletion onelogin_aws_cli/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,18 @@ def _load_config(parser, config_file: ConfigurationFile, args=sys.argv[1:]):

config_section = config_file.section(cli_args.config_name)

if config_section is None:
if config_section is None and cli_args.config_name:
sys.exit(
"Configuration '{}' not defined. "
"Please run 'onelogin-aws-login -c'".format(
cli_args.config_name
)
)
elif config_section is None:
sys.exit(
"No default configuration section found. "
"Please run 'onelogin-aws-login -c"
)

return config_section, cli_args

Expand Down
32 changes: 26 additions & 6 deletions onelogin_aws_cli/configuration.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,32 +7,49 @@
class ConfigurationFile(configparser.ConfigParser):
"""Represents a configuration ini file on disk"""

DEFAULTS = dict(save_password=False)

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

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

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"""
Copy link
Contributor

@drewsonne drewsonne Apr 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@drewsonne drewsonne Apr 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"""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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return len(self.defaults()) > len(self.DEFAULTS)

@property
def is_initialised(self) -> bool:
"""True if there is at least one section"""
"""True if there is at least one section or a defaults section"""
return len(
self.sections()
) > 0
) > 0 or self.has_defaults

def load(self):
self.read_file(self.file)

def initialise(self, config_name='default'):
# 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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

print("It looks like you're using a the deprecated 'default' "
"section.\nConsider renaming the section to 'defaults'.")
self.default_section = 'default'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


def initialise(self, config_name=None):
"""
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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, this got leftover from the merge.

config_name = self.default_section

config_section = self.section(config_name)

config_section['base_uri'] = user_choice(
Expand Down Expand Up @@ -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:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

section_name = self.default_section
if section_name != self.default_section and \
not self.has_section(section_name):
self.add_section(section_name)
return Section(section_name, self)

Expand Down
44 changes: 42 additions & 2 deletions onelogin_aws_cli/tests/test_configurationFile.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,16 @@ def test_can_save_password_username_inherit_false(self):
[profile_test]""")
self.assertTrue(cfg.section("profile_test").can_save_password)

def test_can_save_password_username_defaults_false(self):
cfg = self._helper_build_config("""[defaults]
save_password = false""")
self.assertFalse(cfg.section(None).can_save_password)

def test_can_save_password_username_defaults_true(self):
cfg = self._helper_build_config("""[defaults]
save_password = true""")
self.assertTrue(cfg.section(None).can_save_password)

def test_initialise(self):
str = StringIO()
cfg = ConfigurationFile(str)
Expand All @@ -71,8 +81,6 @@ def test_initialise(self):

self.assertEqual("""[defaults]
save_password = False

[default]
base_uri = https://api.eu.onelogin.com/
client_id = mock_client_id
client_secret = mock_client_secret
Expand All @@ -90,3 +98,35 @@ def test_is_initialised(self):
cf = self._helper_build_config("""[section]
first=foo""")
self.assertTrue(cf.is_initialised)

cf = self._helper_build_config("""[defaults]
first=foo""")
self.assertTrue(cf.is_initialised)

def test_has_defaults(self):

content = StringIO()
cf = ConfigurationFile(content)
self.assertFalse(cf.has_defaults)

cf = self._helper_build_config("""[defaults]
first=foo""")
self.assertTrue(cf.has_defaults)

def test_supports_default(self):

cf = self._helper_build_config("""[defaults]
first=foo""")
self.assertIn("first", cf.defaults())

cf = self._helper_build_config("""[default]
second=bar""")
self.assertIn("second", cf.defaults())

cf = self._helper_build_config("""[defaults]
first=foo

[default]
second=bar""")
self.assertIn("first", cf.defaults())
self.assertNotIn("second", cf.defaults())