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 profiles API version and channel #2062

Merged
merged 4 commits into from
Jun 5, 2023

Conversation

mlavacca
Copy link
Member

What type of PR is this?

/kind gep
/area conformance

What this PR does / why we need it:

In support of #1709, the GEP-1709 has been updated with content on how to detect the gatewayAPIVersion and the gatewayAPIChannel and how to check their consistency.

Does this PR introduce a user-facing change?:

NONE

The GEP-1709 has been updated with content on how to detect the
APIVersion and the channel and how to check their consistency.

Signed-off-by: Mattia Lavacca <[email protected]>
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/gep PRs related to Gateway Enhancement Proposal(GEP) area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 25, 2023
geps/gep-1709.md Outdated Show resolved Hide resolved
Co-authored-by: Shane Utt <[email protected]>
@shaneutt shaneutt self-requested a review May 25, 2023 13:00
Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

/approve
/hold to allow others to take a look
/cc @sunjayBhatia @arkodg @robscott

@k8s-ci-robot k8s-ci-robot requested a review from arkodg May 25, 2023 13:01
@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 25, 2023
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 25, 2023
Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

one issue I could see arising is how to handle an outdated set of CRDs in the cluster in comparison w/ the test suite (e.g. v0.X.Y CRDs vs. running the v1.A.B tests)

I don't expect too much in the way of behavior change between versions going forward since the API is stabilizing, but things that are considered experimental in older versions may no longer be as the tests/versions progress, which could maybe cause a test failure/reporting issue

I wonder if in addition to this we should consider writing in the report what version the test suite is from and discouraging too much version skew (or maybe not allowing any for certified reports)

@shaneutt
Copy link
Member

one issue I could see arising is how to handle an outdated set of CRDs in the cluster in comparison w/ the test suite (e.g. v0.X.Y CRDs vs. running the v1.A.B tests)

I don't expect too much in the way of behavior change between versions going forward since the API is stabilizing, but things that are considered experimental in older versions may no longer be as the tests/versions progress, which could maybe cause a test failure/reporting issue

I wonder if in addition to this we should consider writing in the report what version the test suite is from and discouraging too much version skew (or maybe not allowing any for certified reports)

Test suite version tracking is a good point. I think we could incorporate that here, unless you'd like to do a follow up PR to add this and any other thoughts you have on the proposal so far @sunjayBhatia?

@sunjayBhatia
Copy link
Member

Test suite version tracking is a good point. I think we could incorporate that here, unless you'd like to do a follow up PR to add this and any other thoughts you have on the proposal so far @sunjayBhatia?

If @mlavacca wants to add it here that's great, otherwise I'm happy to make a follow-up 👍🏽

@mlavacca
Copy link
Member Author

mlavacca commented May 26, 2023

If @mlavacca wants to add it here that's great, otherwise I'm happy to make a follow-up 👍🏽

Thanks for this good point @sunjayBhatia, I've addressed it here

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 26, 2023
Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

looks great @mlavacca!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkodg, mlavacca, shaneutt, sunjayBhatia

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

@shaneutt shaneutt removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 26, 2023
@shaneutt
Copy link
Member

Thanks for the review @arkodg and @sunjayBhatia!

I'll let one of my co-maintainers drop the final LGTM:

/cc @robscott @youngnick

Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @mlavacca! Mostly LGTM, just some questions.

@@ -280,6 +318,7 @@ implementation:
- @acme/maintainers
date: "2023-02-28 20:29:41+00:00"
gatewayAPIVersion: v0.7.0
gatewayAPIChannel: standard
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that most implementations would need to submit separate reports for each Gateway API Channel? I'm not sure we gain any value from that since the experimental channel is always a superset of the standard channel, but I may be missing something here.

I agree that the version of tests that are being run is critical though, so I'm glad that's included here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since the experimental channel is a superset of the standard one, we could treat the report in the same way, which means being conformant to the experimental channel implies being conformant to the standard one. So that you just need to submit the experimental report to declare the standard conformance as well. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

That's kinda what I'm thinking - we may just want to recommend that implementations that support experimental features run conformance tests with experimental CRDs. I'm not sure that we need to actually state that in the report, but don't feel strongly either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I agree we need to add the proper documentation about this as well.

geps/gep-1709.md Show resolved Hide resolved
geps/gep-1709.md Outdated Show resolved Hide resolved
Signed-off-by: Mattia Lavacca <[email protected]>
@mlavacca
Copy link
Member Author

/retest

@robscott
Copy link
Member

robscott commented Jun 5, 2023

Thanks @mlavacca!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2023
@k8s-ci-robot k8s-ci-robot merged commit e3f9955 into kubernetes-sigs:main Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/gep PRs related to Gateway Enhancement Proposal(GEP) lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants