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

chore(conformance): shorten the resource name #2589

Closed

Conversation

tao12345666333
Copy link
Member

prevent it from destroying other projects

What type of PR is this?

/area conformance
/kind test
What this PR does / why we need it:

fails with contour since the gateway name has a really long name and that causes a label we add to some resources to be > 63 chars

selfishly it would be great to have a shorter gateway name here but otherwise the logic of the test looks good

test passes with contour when i shorten the gateway name
_Originally posted by @sunjayBhatia #2582 (review)

In Kubernetes, 63 is a very important length.
Although for Gateway resources, its name can exceed 63, different implementations may use it in different positions.

I think conformance tests should be as universal as possible to avoid encountering problems like this.
I also checked other test cases and currently, all lengths are below 50.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

None

to avoid reaching the limit of 63 characters.

Signed-off-by: Jintao Zhang <[email protected]>
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. area/conformance kind/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 17, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tao12345666333
Once this PR has been reviewed and has the lgtm label, please assign howardjohn for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 17, 2023
Copy link
Member

@mlavacca mlavacca left a comment

Choose a reason for hiding this comment

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

I'm not sure about this change, to be honest. I think that implementations should deal with this kind of problem and they should not expect only gateway names which length is <= 50 chars. I get the point of not destroying other implementations, but since we just released v1.0 and there likely is some time before the next release, I am not sure about mitigating this issue, since it's an implementation issue rather than an API one. @sunjayBhatia How hard is it for you to fix this problem in Contour? Do you think you'll be able to fix it in a short time or do you foresee potential compatibility issues?

I want to underline I'm 100% down on "I do not want to break implementations", and if this can happen I'm okay with this change.

@sunjayBhatia
Copy link
Member

I'm not sure about this change, to be honest. I think that implementations should deal with this kind of problem and they should not expect only gateway names which length is <= 50 chars. I get the point of not destroying other implementations, but since we just released v1.0 and there likely is some time before the next release, I am not sure about mitigating this issue, since it's an implementation issue rather than an API one. @sunjayBhatia How hard is it for you to fix this problem in Contour? Do you think you'll be able to fix it in a short time or do you foresee potential compatibility issues?

I want to underline I'm 100% down on "I do not want to break implementations", and if this can happen I'm okay with this change.

We have an issue to fix this in contour already: projectcontour/contour#5970

I mainly just called it out for completeness since I had to modify the tests while reviewing to get them to pass properly with contour

@mlavacca
Copy link
Member

I'm not sure about this change, to be honest. I think that implementations should deal with this kind of problem and they should not expect only gateway names which length is <= 50 chars. I get the point of not destroying other implementations, but since we just released v1.0 and there likely is some time before the next release, I am not sure about mitigating this issue, since it's an implementation issue rather than an API one. @sunjayBhatia How hard is it for you to fix this problem in Contour? Do you think you'll be able to fix it in a short time or do you foresee potential compatibility issues?
I want to underline I'm 100% down on "I do not want to break implementations", and if this can happen I'm okay with this change.

We have an issue to fix this in contour already: projectcontour/contour#5970

I mainly just called it out for completeness since I had to modify the tests while reviewing to get them to pass properly with contour

This is great! I guess this change is not needed anymore?

@sunjayBhatia
Copy link
Member

This is great! I guess this change is not needed anymore?

Yeah I would say so 👍🏽 we can come back to this if for whatever reason we can't solve the issue in contour, but that is unlikely

@mlavacca
Copy link
Member

This is great! I guess this change is not needed anymore?

Yeah I would say so 👍🏽 we can come back to this if for whatever reason we can't solve the issue in contour, but that is unlikely

👍 Thanks anyway @tao12345666333 for promptly jumping on it.

/close

@k8s-ci-robot
Copy link
Contributor

@mlavacca: Closed this PR.

In response to this:

This is great! I guess this change is not needed anymore?

Yeah I would say so 👍🏽 we can come back to this if for whatever reason we can't solve the issue in contour, but that is unlikely

👍 Thanks anyway @tao12345666333 for promptly jumping on it.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tao12345666333
Copy link
Member Author

Make sense. Thanks for reply.

@sunjayBhatia
Copy link
Member

sunjayBhatia commented Nov 17, 2023

@tao12345666333 @mlavacca added #2592 since this actually has wider implications beyond just this test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/test release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants