Add `all-features` flag to add ability to enable all supported feature conformance tests. #1642

gyohuangxin · 2023-01-11T08:12:42Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

It adds all-features flag to add ability to enable all supported feature conformance tests. For example:

go test ./conformance --gateway-class istio --debug --all-features

Which issue(s) this PR fixes:

Fixes #1638

Does this PR introduce a user-facing change?:

1. Add `all-features` flag to enable all supported feature conformance tests.

gyohuangxin · 2023-01-11T08:39:26Z

@robscott @arkodg Could you review this PR? Thanks.

arkodg · 2023-01-11T20:23:18Z

conformance/utils/flags/flags.go

+	CleanupBaseResources       = flag.Bool("cleanup-base-resources", true, "Whether to cleanup base test resources after the run")
+	SupportedFeatures          = flag.String("supported-features", "", "Supported features included in conformance tests suites")
+	ExemptFeatures             = flag.String("exempt-features", "", "Exempt Features excluded from conformance tests suites")
+	EnableAllSupportedFeatures = flag.Bool("all-features", false, "Whether to enable all supported feature conformance tests")


flag name lgtm !

arkodg · 2023-01-11T20:25:11Z

conformance/utils/suite/suite.go

@@ -77,6 +77,23 @@ var StandardCoreFeatures = map[SupportedFeature]bool{
 	SupportReferenceGrant: true,
 }

+// AllFeatures contains all the supported features and can be used to run all
+// conformance tests with `all-features` flag.
+var AllFeatures = map[SupportedFeature]bool{


is there any way a contributor defines the suite name once ? I fear the contributor will add it the constant w/o adding it to this map, and the burden to ensure its in sync will fall on the maintainer/ code reviewer

Yes, you are right, developers need to modify two places after the constant changes. But I can't find a simple way to define the suite name once in this codebase. Or I can add a comment to notice developer/code reviewer to ensure they are in sync?
@robscott Can you help with some advice?

I can't think of a better solution rn, can you add a comment to ensure they are in sync, TIA !

gyohuangxin · 2023-01-17T14:28:28Z

@robscott Could you review it? Thanks

conformance/utils/flags/flags.go

shaneutt · 2023-01-17T15:24:00Z

conformance/utils/suite/suite.go

@@ -77,6 +77,23 @@ var StandardCoreFeatures = map[SupportedFeature]bool{
 	SupportReferenceGrant: true,
 }

+// AllFeatures contains all the supported features and can be used to run all
+// conformance tests with `all-features` flag.
+var AllFeatures = map[SupportedFeature]bool{


Perhaps we should consider using a set for this instead of a map?

IMO, there is no native set structure in golang, do you mean the map[SupportedFeature]struct{}? Also we should pass AllFeatures to suite's SupportedFeatures, so we should make them the same type.

Sets are available in apimachinery:

https://github.com/kubernetes/apimachinery/tree/master/pkg/util/sets

e.g.:

import "k8s.io/apimachinery/pkg/utils/sets" var AllFeatures = sets.NewString( SupportReferenceGrant, SupportTLSRoute, SupportHTTPRouteQueryParamMatching, SupportHTTPRouteMethodMatching, SupportHTTPResponseHeaderModification, SupportRouteDestinationPortMatching, SupportGatewayClassObservedGenerationBump, SupportHTTPRoutePortRedirect, SupportHTTPRouteSchemeRedirect, SupportHTTPRoutePathRedirect, SupportHTTPRouteHostRewrite, SupportHTTPRoutePathRewrite, )

if AllFeatures.Has(SupportReferenceGrant) {

@shaneutt Thank you for providing this example, it does provide an easy way to use set.
However, var AllFeatures = map[SupportedFeature]bool is also equivalent to a set, and I think it's better to keep AllFeatures declared in the same way as StandardCoreFeatures unless we also change the declaration method of StandardCoreFeatures. What do you think? Please correct me if I was wrong.

Yes the implementation ultimately uses a map, so the benefit here is for the developers which I think is a fair thing to trade for, however I don't consider this suggestion a blocker I'll let others weigh in.

Understood, if others also want the benefit, I can implement it.

Now that the k8s sets util supports generics, I think it would be best to move both this and AllFeatures over to use that approach.

Now that the k8s sets util supports generics, I think it would be best to move both this and AllFeatures over to use that approach.

@robscott @shaneutt I tried to move it to K8s sets utils: b85174a, can you review if it is correct?

robscott

Thanks @gyohuangxin! Agree with moving to k8s sets util, but otherwise this LGTM.

robscott · 2023-01-20T18:48:04Z

conformance/utils/suite/suite.go

@@ -77,6 +77,23 @@ var StandardCoreFeatures = map[SupportedFeature]bool{
 	SupportReferenceGrant: true,
 }

+// AllFeatures contains all the supported features and can be used to run all
+// conformance tests with `all-features` flag.
+var AllFeatures = map[SupportedFeature]bool{


Now that the k8s sets util supports generics, I think it would be best to move both this and AllFeatures over to use that approach.

arkodg

lgtm, thanks for adding this !

…e conformance tests. Signed-off-by: Huang Xin <[email protected]>

Signed-off-by: Huang Xin <[email protected]>

…features when feature constants change. Signed-off-by: Huang Xin <[email protected]>

Signed-off-by: Huang Xin <[email protected]>

conformance/utils/suite/suite.go

Signed-off-by: Huang Xin <[email protected]>

shaneutt

Would like @robscott to give the final LGTM since he's been reviewing this too, but does LGTM.

conformance/utils/suite/suite.go

Signed-off-by: Huang Xin <[email protected]>

gyohuangxin · 2023-02-02T05:24:45Z

@robscott Can you review it?

robscott

Thanks @gyohuangxin!

conformance/utils/suite/suite.go

Co-authored-by: Rob Scott <[email protected]>

shaneutt

/lgtm

k8s-ci-robot · 2023-02-06T18:56:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkodg, gyohuangxin, 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:

~~conformance/OWNERS~~ [shaneutt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Add `all-features` flag to add ability to enable all supported feature conformance tests.

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 11, 2023

k8s-ci-robot requested review from bowei and howardjohn January 11, 2023 08:12

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 11, 2023

arkodg reviewed Jan 11, 2023

View reviewed changes

shaneutt reviewed Jan 17, 2023

View reviewed changes

shaneutt requested a review from robscott January 20, 2023 10:27

robscott reviewed Jan 20, 2023

View reviewed changes

arkodg approved these changes Jan 24, 2023

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 26, 2023

gyohuangxin added 4 commits January 28, 2023 09:39

Add all-features flag to add ability to enable all supported featur…

2a83610

…e conformance tests. Signed-off-by: Huang Xin <[email protected]>

Fix format.

b03c074

Signed-off-by: Huang Xin <[email protected]>

Fix format.

9dbea10

Signed-off-by: Huang Xin <[email protected]>

Add the comment to note AllFeatures must be in sync with the defined …

2cc158b

…features when feature constants change. Signed-off-by: Huang Xin <[email protected]>

gyohuangxin force-pushed the all-features-conformance-flag branch from 173f08e to 2cc158b Compare January 28, 2023 01:40

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2023

Fix API error.

4d558b8

Signed-off-by: Huang Xin <[email protected]>

gyohuangxin force-pushed the all-features-conformance-flag branch 2 times, most recently from d4a4440 to b85174a Compare January 28, 2023 07:00

Move to K8s sets util.

b85174a

Signed-off-by: Huang Xin <[email protected]>

shaneutt requested review from robscott, shaneutt and arkodg January 30, 2023 14:11

shaneutt reviewed Jan 30, 2023

View reviewed changes

conformance/utils/suite/suite.go Outdated Show resolved Hide resolved

Use sets.New

7c18de2

Signed-off-by: Huang Xin <[email protected]>

gyohuangxin removed the request for review from robscott January 31, 2023 12:27

gyohuangxin requested review from shaneutt and removed request for howardjohn, bowei and arkodg January 31, 2023 12:27

shaneutt requested a review from robscott January 31, 2023 13:38

shaneutt approved these changes Jan 31, 2023

View reviewed changes

conformance/utils/suite/suite.go Outdated Show resolved Hide resolved

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2023

Reduce replicute codes after using sets.

3cf4e7d

Signed-off-by: Huang Xin <[email protected]>

shaneutt approved these changes Jan 31, 2023

View reviewed changes

shaneutt added this to the v0.6.1 milestone Feb 3, 2023

robscott reviewed Feb 3, 2023

View reviewed changes

conformance/utils/suite/suite.go Outdated Show resolved Hide resolved

Update conformance/utils/suite/suite.go

d2a9e13

Co-authored-by: Rob Scott <[email protected]>

shaneutt approved these changes Feb 6, 2023

View reviewed changes

k8s-ci-robot assigned shaneutt Feb 6, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 6, 2023

k8s-ci-robot merged commit d8b8994 into kubernetes-sigs:main Feb 6, 2023

shaneutt pushed a commit that referenced this pull request Feb 7, 2023

Merge pull request #1642 from gyohuangxin/all-features-conformance-flag

4c7a0fa

Add `all-features` flag to add ability to enable all supported feature conformance tests.

arkodg mentioned this pull request Mar 7, 2023

REQUEST: New membership for arkodg kubernetes/org#4081

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `all-features` flag to add ability to enable all supported feature conformance tests. #1642

Add `all-features` flag to add ability to enable all supported feature conformance tests. #1642

gyohuangxin commented Jan 11, 2023

gyohuangxin commented Jan 11, 2023

arkodg Jan 11, 2023

arkodg Jan 11, 2023 •

edited

Loading

gyohuangxin Jan 12, 2023

arkodg Jan 17, 2023

gyohuangxin commented Jan 17, 2023

shaneutt Jan 17, 2023

gyohuangxin Jan 19, 2023 •

edited

Loading

shaneutt Jan 19, 2023

gyohuangxin Jan 19, 2023

shaneutt Jan 19, 2023

gyohuangxin Jan 20, 2023

robscott Jan 20, 2023

gyohuangxin Jan 28, 2023

robscott left a comment

robscott Jan 20, 2023

arkodg left a comment

shaneutt left a comment

gyohuangxin commented Feb 2, 2023

robscott left a comment

shaneutt left a comment

k8s-ci-robot commented Feb 6, 2023

Add all-features flag to add ability to enable all supported feature conformance tests. #1642

Add all-features flag to add ability to enable all supported feature conformance tests. #1642

Conversation

gyohuangxin commented Jan 11, 2023

gyohuangxin commented Jan 11, 2023

Choose a reason for hiding this comment

arkodg Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyohuangxin commented Jan 17, 2023

Choose a reason for hiding this comment

gyohuangxin Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkodg left a comment

Choose a reason for hiding this comment

shaneutt left a comment

Choose a reason for hiding this comment

gyohuangxin commented Feb 2, 2023

robscott left a comment

Choose a reason for hiding this comment

shaneutt left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 6, 2023

Add `all-features` flag to add ability to enable all supported feature conformance tests. #1642

Add `all-features` flag to add ability to enable all supported feature conformance tests. #1642

arkodg Jan 11, 2023 •

edited

Loading

gyohuangxin Jan 19, 2023 •

edited

Loading