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
fixup(sg-resolver): Allow multiple SGs with the same Name tag #3775
fixup(sg-resolver): Allow multiple SGs with the same Name tag #3775
Changes from 1 commit
41f7943
12a7953
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.
do we need to add nil check for the
sgs
andsg.Tags
?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 think the Golang specification tells us that these would be unnecessarily redundant.
Source: https://go.dev/ref/spec#For_range
Example: https://go.dev/play/p/vBe3S9lbhvh
That said, if you feel strongly about having them or is a common pattern in the codebase I am more than happy to add an extra commit with the additional
nil
checks 😉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.
yeah for the
sgs
it was fine, but how aboutsg.Tags
? I know in this function we are filtering by tags, so the returned sgs should haveTags
field, but just in case if some code changes in the future and we miss this, it will error out because the controller does not check whether there's a Tag spec.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 think for
sg.Tags
the reasoning is the the same 🤔? Also, if the listing code changes and the returnedSecurityGroup
struct removes/renames theTags
field, that will produce a compiler error and, hence, we will detect it immediately. Am I missing something?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.
can we keep a negative test case for the mix name&ids ?
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 pushed 12a7953 to add the negative test for mix name & ids as well as improve error reporting a little bit.