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

Reject unrecognized options in config objects (but still allow it at the top-level) #623

Merged
merged 5 commits into from
Jul 8, 2018

Conversation

danielkza
Copy link
Contributor

No description provided.

Copy link
Member

@phobologic phobologic left a comment

Choose a reason for hiding this comment

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

Mostly some questions, and possibly could use another test (though may be already covered, let me know). Thanks!

try:
return Config(config_dict, strict=True)
except SchematicsError as e:
raise exceptions.InvalidConfig(e.errors)
Copy link
Member

Choose a reason for hiding this comment

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

I don't know the schematics Exceptions well enough - but will this provide enough data to figure out what the issue is? I really wish python2 provided a good way for wrapping exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the only issue is the message is not very explanatory. e.errors is something like this when an invalid key is present: {"stacks": {"0": {"garbage": "Rogue field"}}}. But I think the best place to fix it is in Schematics itself.

@@ -537,6 +537,20 @@ def test_raise_construct_error_on_duplicate_stack_name_dict(self):
with self.assertRaises(ConstructorError):
parse(yaml_config)

def test_parse_invalid_inner_keys(self):
Copy link
Member

Choose a reason for hiding this comment

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

Do we already have a test that has excess keys at the top level that would cover that use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thanks!

@phobologic phobologic added this to the 1.4 milestone Jul 8, 2018
@phobologic phobologic merged commit f21a6ea into cloudtools:master Jul 8, 2018
@phobologic
Copy link
Member

Thanks @danielkza !

@danielkza danielkza deleted the strict-parsing branch July 8, 2018 03:20
phrohdoh pushed a commit to phrohdoh/stacker that referenced this pull request Dec 18, 2018
…the top-level) (cloudtools#623)

* config: re-enable strict mode by manually removing excess top level keys

* tests: fix typo in stack_policy_path in functional tests

* config: ensure Schematics errors are converted during parsing

Validation already caught them, but parsing needs to do it too.

* Update changelog with strict parsing changes

* tests: fix stack policy path (should be relative to the tests dir)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants