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

Fix traceback on interactive provider when adding resources #198

Conversation

danielkza
Copy link
Contributor

It seems it's possible for resources to omit the 'Replacement' key
instead of setting it to false. Handle that case instead of assuming the
key is always present.

Fixes a traceback like the following:

Traceback (most recent call last):
  File "/home/danielkza/bin/stacker", line 6, in <module>
    exec(compile(open(__file__).read(), __file__, 'exec'))
  File "/home/danielkza/src/stacker/scripts/stacker", line 9, in <module>
    args.run(args)
  File "/home/danielkza/src/stacker/stacker/commands/stacker/build.py", line 52, in run
    dump=options.dump)
  File "/home/danielkza/src/stacker/stacker/actions/base.py", line 125, in execute
    self.run(*args, **kwargs)
  File "/home/danielkza/src/stacker/stacker/actions/build.py", line 301, in run
    plan.execute()
  File "/home/danielkza/src/stacker/stacker/plan.py", line 251, in execute
    if not self._single_run():
  File "/home/danielkza/src/stacker/stacker/plan.py", line 219, in _single_run
    status = step.run()
  File "/home/danielkza/src/stacker/stacker/plan.py", line 72, in run
    return self._run_func(self.stack, status=self.status)
  File "/home/danielkza/src/stacker/stacker/actions/build.py", line 220, in _launch_stack
    tags)
  File "/home/danielkza/src/stacker/stacker/providers/aws/interactive.py", line 164, in update_stack
    replacements_only=self.replacements_only)
  File "/home/danielkza/src/stacker/stacker/providers/aws/interactive.py", line 83, in output_summary
    replacement = resource['Replacement'] == 'True'
KeyError: 'Replacement'

@@ -80,7 +80,7 @@ def output_summary(fqn, action, changeset, replacements_only=False):
changes = []
for change in changeset:
resource = change['ResourceChange']
replacement = resource['Replacement'] == 'True'
replacement = resource.get('Replacement', '') == 'True'
Copy link
Member

Choose a reason for hiding this comment

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

Only a minor nit: You can just do resource.get('Replacement') == 'True'

.get() will return None by default if the key doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I got into the habit of making it explicit for clarity, and I thought it would maybe be better to return a string for uniformity, but it's only a matter of preference, and keeping it like the rest of the codebase is best.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah - I think I prefer just letting it use the usual default, unless there's actually something useful about specifying a new default. If you can update that, I'll pull this in ASAP :) Thanks @danielkza

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@phobologic Done.

@phobologic
Copy link
Member

This looks good - one minor change, and I'll go ahead and merge. Thanks @danielkza !

It seems it's possible for resources to omit the 'Replacement' key
instead of setting it to false. Handle that case instead of assuming the
key is always present.
@danielkza danielkza force-pushed the fix-interactive-provider-exception-on-resource-add branch from 94afae1 to 4b2e8bf Compare August 29, 2016 02:24
@phobologic phobologic merged commit 02a5ad5 into cloudtools:master Aug 29, 2016
@phobologic
Copy link
Member

Thanks!

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