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.

Sign up for GitHub

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

Jump to bottom

Conformance Tests: No implicit test runs without features #1894

Merged
merged 4 commits into from
Apr 4, 2023
Merged

Conformance Tests: No implicit test runs without features #1894

merged 4 commits into from
Apr 4, 2023

Conversation

shaneutt
Copy link
Member

@shaneutt shaneutt commented Mar 31, 2023 •
edited
Loading

What type of PR is this?

/kind cleanup
/kind test
/area conformance

What this PR does / why we need it:

In #1890 we removed implicit support for HTTPRoute, and created #1891 as a follow up to do this for all types. With #1878 bringing Mesh conformance tests soon this patch makes it cleaner for implementations which ONLY support mesh to simply not run any unrelated conformance tests.

Which issue(s) this PR fixes:

This doesn't completely fix, but does support #1891.

Does this PR introduce a user-facing change?:

No conformance tests run by default anymore, including tests for GatewayClass and Gateway. A new
SupportGateway feature must be opted into in order to run those tests (similar to what we've done
previously for ReferenceGrant and HTTPRoute). Also with this release, `EnableAllSupportedFeatures`
enables all Gateway AND Mesh features (where previously that was just Gateway).

@shaneutt shaneutt added the release-blocker MUST be completed to complete the milestone label Mar 31, 2023
@shaneutt shaneutt added this to the v0.7.0 milestone Mar 31, 2023
@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/test area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 31, 2023
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 31, 2023
@shaneutt shaneutt mentioned this pull request Mar 31, 2023
Merged
@shaneutt shaneutt added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Mar 31, 2023
@shaneutt
Copy link
Member Author

/test all

@shaneutt shaneutt force-pushed the shaneutt/mesh-conformance-machinery-patch branch from 751d34a to 79e23c9 Compare April 3, 2023 16:59
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 3, 2023
@shaneutt
chore: rename gamma -> mesh in conformance context
b5529c1 
GAMMA is an acronym for the project which is working on
Gateway API resources for service mesh use cases. As such
when it comes to conformance, we're not really dealing
with "Gamma" features we're dealing with "service mesh"
features.
@shaneutt
chore: remove mesh specific conformance test list
6539ffe
@shaneutt
feat: all tests require features
79e23c9 
This patch ensures that NO tests run without a feature enabled
for them. This allows for better differentiation for implementations
which might support (for instance) only mesh, and is done in
preparation for conformance profiles.
@shaneutt shaneutt marked this pull request as ready for review April 3, 2023 17:01
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 3, 2023
arkodg
arkodg approved these changes Apr 3, 2023
View reviewed changes
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM !

@candita
Copy link
Contributor

candita commented Apr 3, 2023

@shaneutt could you add a quick update to the conformance docs in https://github.com/kubernetes-sigs/gateway-api/blob/main/site-src/concepts/conformance.md ?

@arkodg
Copy link
Contributor

arkodg commented Apr 3, 2023

curious will EnableAllSupportedFeatures run Gateway and Mesh tests going forward ?

@howardjohn
Copy link
Contributor

EnableAllSupportedFeatures already runs mesh before this change. If you don't want mesh, then you don't want all features I think. maybe we should have AllGatewayFeatures helper though?

@arkodg
Copy link
Contributor

arkodg commented Apr 3, 2023 •
edited
Loading

EnableAllSupportedFeatures already runs mesh before this change. If you don't want mesh, then you don't want all features I think. maybe we should have AllGatewayFeatures helper though?

thanks for the clarification, in that case a release note mentioning this would be good to have.
and maybe that EnableAllSupportedFeatures can be replaced with just enabling SupportGateway feature suite, to retain previous intent ?

@shaneutt
Copy link
Member Author

shaneutt commented Apr 4, 2023

@shaneutt could you add a quick update to the conformance docs in https://github.com/kubernetes-sigs/gateway-api/blob/main/site-src/concepts/conformance.md ?

Good call! changed in 2acae49 👍

@shaneutt
Copy link
Member Author

shaneutt commented Apr 4, 2023

EnableAllSupportedFeatures already runs mesh before this change. If you don't want mesh, then you don't want all features I think. maybe we should have AllGatewayFeatures helper though?

thanks for the clarification, in that case a release note mentioning this would be good to have.

I had already had a release note in the PR description, let me know if you feel it needs to be expanded upon.

@arkodg
Copy link
Contributor

arkodg commented Apr 4, 2023 •
edited
Loading

EnableAllSupportedFeatures already runs mesh before this change. If you don't want mesh, then you don't want all features I think. maybe we should have AllGatewayFeatures helper though?

thanks for the clarification, in that case a release note mentioning this would be good to have.

I had already had a release note in the PR description, let me know if you feel it needs to be expanded upon.

thanks, this is fine for this PR, but related to #1878, would be good to add another section in the release note saying - "With this release, EnableAllSupportedFeatures enables all Gateway and Mesh features. If you are interested in only running Gateway feature tests, we recommend no longer using EnableAllSupportedFeatures

michaelbeaumont
michaelbeaumont approved these changes Apr 4, 2023
View reviewed changes
Copy link
Contributor

@michaelbeaumont michaelbeaumont left a comment

Choose a reason for hiding this comment

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

LGTM! +1 on making the release notes very explicit

@shaneutt
Copy link
Member Author

shaneutt commented Apr 4, 2023

Release notes updated as requested, thank you! 👍

robscott
robscott approved these changes Apr 4, 2023
View reviewed changes
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 @shaneutt!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 4, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkodg, michaelbeaumont, robscott, shaneutt

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

@k8s-ci-robot k8s-ci-robot merged commit ccc922b into kubernetes-sigs:main Apr 4, 2023
@shaneutt shaneutt mentioned this pull request Apr 5, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robscott robscott robscott approved these changes

@michaelbeaumont michaelbeaumont michaelbeaumont approved these changes

@arkodg arkodg arkodg approved these changes

@howardjohn howardjohn Awaiting requested review from howardjohn

@mikemorris mikemorris Awaiting requested review from mikemorris

@Xunzhuo Xunzhuo Awaiting requested review from Xunzhuo

@mlavacca mlavacca Awaiting requested review from mlavacca

@skriss skriss Awaiting requested review from skriss

@sunjayBhatia sunjayBhatia Awaiting requested review from sunjayBhatia

Assignees

@robscott robscott

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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/test lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-blocker MUST be completed to complete the milestone size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
No open projects
Gateway API: The Road to GA
Status: Done
Milestone
v0.7.0
Development

Successfully merging this pull request may close these issues.
7 participants
@shaneutt @k8s-ci-robot @candita @arkodg @howardjohn @robscott @michaelbeaumont