-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: add support to set ProtocolVersion based on annotation values #1589
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
Conversation
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.
Thanks for add support for this.
overall looks good. changes needed in addition to the inline comments 😄:
-
we should change the
buildTargetGroupName
in Ingress's model builder to have a protocolVersion passed-in, and use it as a hash component. (since protocolVersion is immutable, we need to create a targetGroup with different name if ppl changes it). -
we should change the
isSDKTargetGroupRequiresReplacement
indeployment/elbv2/target_group_synthesizer.go
to be like
if resTG.Spec.ProtocolVersion != nil {
if awssdk.StringValue(resTG.Spec.ProtocolVersion) != awssdk.SrtingValue(sdkTG.TargetGroup.ProtocolVersion) {
return true
}
}
sounds good - will tackle these shortly. |
Addressed the review comments |
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.
/lgtm
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: backjo, M00nF1sh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
thanks soo much dude, looks perfect now 👍 |
…ubernetes-sigs#1589) * feat: add support to set ProtocolVersion based on annotation values * Add comment to mock method * address review comments * Fix typo
Closes #1586
This change is