-
Notifications
You must be signed in to change notification settings - Fork 68
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
Create Ingress2gateway version annotation and attach it to all Gateway resources #187
Create Ingress2gateway version annotation and attach it to all Gateway resources #187
Conversation
/assign @LiorLieberman |
Would like some suggestion on the preferred format of this kind of annotation. I created the current one based on how ingressnginx specify proxy http version https://github.com/kubernetes/ingress-nginx/blob/main/docs/user-guide/nginx-configuration/annotations.md#proxy-http-version.
|
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 @sawsa307!
1742057
to
6241bd5
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: robscott, sawsa307 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Hi @sawsa307!
Thank you for your contribution!
I have one small comment in case we are going for this change. Since this feature requires manual version modification upon a new release, please update the RELEASE.md
file with the appropriate steps.
6241bd5
to
ae55497
Compare
Thanks for the comment Kobi! I've just added a section in RELEASE.md. Let me know if you have any suggestions on it, thank you! |
ae55497
to
357037b
Compare
Is there a reason we do it only for gateway? |
We could attach this in other resources like HTTPRoute, but I don't think we can gain any information with it since we can find all translated HTTPRoute based on the Gateways link to them. |
Actually I think this is a good point - might as well add this annotation to all generated resources so it's both more consistent and easier to keep track of where and GW API resource came from. |
357037b
to
909ec9d
Compare
909ec9d
to
75098fe
Compare
Sure, just attaches this annotation on all translated Gateway resources. |
@sawsa307 The release note mention |
resources. * Ingress2gateway annotation will track the current version of the ingress2gateway tool. The version is currently a string that needs to be updated manually before release. * This annotation will be attached to the resulting Gateway resources during print since this is an output-only translation, and we can avoid interfering with existing Gateway translation tests.
I updated the release note myself to unblock. /lgtm |
75098fe
to
9598576
Compare
It should be updated now @LiorLieberman.
|
/lgtm |
Example output:
What type of PR is this?
/kind feature
What this PR does / why we need it:
Create a dedicated annotation for Gateways translated via ingress2gateway tool to keep track of resources that we're generated with ingress2gatewa
Which issue(s) this PR fixes:
Fixes #179
Does this PR introduce a user-facing change?: