Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(ec2): user-defined subnet selectors #10112
feat(ec2): user-defined subnet selectors #10112
Changes from 9 commits
446a54c
61c6f96
638ec87
c1cd7c0
8ba3b7b
48d6b56
9286b0e
be6dabd
d622b19
0cc51f7
32fdd47
5609f93
59af473
4fb96e2
724b2a5
d0ca7c3
d691e0c
84c6ae1
cb5935b
68a6b3b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this at least lead to the appropriate filter class being prepended to the list?
Strictly speaking this method is
protected
, therefore has a public contract to other people that could have inherited from this class and are relying on its current behavior.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you're correct. That functionality is now in
reifySelectionDefaults
. I did the same for the one-per-az selection option.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is API contract misuse and should definitely remain an error that aborts the program.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error still occurs if the user specifies both, like before. Now the user will get a friendly reminder that
subnetName
shouldn't be used, but still allows them to use it -- like before.Should I change the behavior such that it throws an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, I misread.
Well, hmm. I guess it's maybe a little overkill, but fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This leaves the existing
.availabilityZones
and.onePerAz
booleans in place.Are you sure they're not going to be interpreted again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did remove their interpretation everywhere else but here, but I suppose there's no harm in removing them completely.