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(aws): Alter AllowLaunchDescription to not overload account #4021

Merged
merged 1 commit into from Sep 11, 2019

Conversation

robzienert
Copy link
Member

We asked, "What could possibly go wrong?" with #4017. It turns out AllowLaunchDescription overloaded account, which broke any deploy from a newly baked image. That's what can go wrong.

This PR fixes that, making the description use a new field and fix-forward whatever Orca sends us.

Copy link
Contributor

@cfieber cfieber left a comment

Choose a reason for hiding this comment

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

just the one comment otherwise LGTM

public Map process(Map description) {
// Backwards-compatibility from when AllowLaunch used to overload "account" from the abstract
// AWS description.
description.put("targetAccount", description.get("account"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd propose for this to do a putIfAbsent("targetAccount") instead otherwise callers will have to continue to supply account once they have been updated

Copy link
Member Author

Choose a reason for hiding this comment

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

Good one. OK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants