Implement TEP-0033: Add enable-api-fields feature-flag #3881

ghost · 2021-04-14T16:26:21Z

Changes

Implements TEP-0033:
https:/tektoncd/community/blob/main/teps/0033-tekton-feature-gates.md

This commit introduces a new feature flag, "enable-api-fields", that
can be set to "alpha" or "stable". The default is "stable".

This commit provides:

dev documentation describing how to check the feature gate and use the provided tools during testing
a helper for performing feature gate validation in our validating webhook
a test helper, requireGate that allows an integration test to be easily skipped if "enable-api-fields" is not "alpha".
a way to run YAML examples specific to each feature-gate:

New examples can be added under a directory called "alpha". When
the "enable-api-fields" flag is set to "alpha" the test runner will
only run those tests in "alpha" directories. Similarly if the flag is
"stable" then the examples under "alpha" will be prevented from running.
See the Step and Sidecar Workspaces branch for examples that use this
new mechanism:

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

A new feature-flag, "enable-api-fields" has been added. Valid values are "alpha" and "stable". It defaults to "stable".

Setting the "enable-api-fields" flag tells Tekton what level of API stability you require in the cluster. As new features are added to Pipelines we'll first place those features under the "alpha" flag. When the feature is ready we'll promote it to "stable". This process is described in TEP-0033[1].

Opting in to "alpha" gives you early exposure to exciting new features as they're added to Pipelines but those features are still undergoing development and could be subject to backwards-incompatible changes.

[1] https:/tektoncd/community/blob/main/teps/0033-tekton-feature-gates.md

ghost · 2021-04-14T16:26:42Z

/kind feature

tekton-robot · 2021-04-14T16:28:21Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	71.0%	76.3%	5.3

tekton-robot · 2021-04-14T16:53:14Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	71.0%	76.3%	5.3

tekton-robot · 2021-04-14T17:04:51Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	71.0%	76.3%	5.3

tekton-robot · 2021-04-14T17:48:51Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	71.0%	76.3%	5.3
test/gate.go	Do not exist	0.0%

vdemeester

Looks very good to me 🙃

vdemeester · 2021-04-16T03:50:17Z

test/gate.go

+// test if the feature-flag with given name does not equal
+// given value. It will fatally fail the test if it cannot get
+// the feature-flag configmap.
+func requireGate(name, value string) func(context.Context, *testing.T, *clients, string) {


ghost · 2021-04-16T11:47:44Z

TODO:

Add words to DEVELOPMENT.md to tell devs how to use these things when writing new features
Figure out how to conditionally Validate alpha features / deny submission when an alpha feature is used in "stable" environment.

ghost · 2021-04-16T11:48:03Z

/hold

Still work to be done on this.

tekton-robot · 2021-04-16T13:20:22Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	71.0%	76.3%	5.3
test/gate.go	Do not exist	0.0%

ghost · 2021-04-16T18:50:45Z

Updated with a validation function that can be used by alpha API fields to guard them from being used when enable-api-fields is stable.

tekton-robot · 2021-04-16T18:52:45Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	71.0%	76.3%	5.3
pkg/apis/pipeline/v1beta1/version_validation.go	Do not exist	100.0%
test/gate.go	Do not exist	0.0%

ghost · 2021-04-16T19:32:29Z

And now also updated with developer docs explaining how to use these new tools while writing feature-guarded code. I think this is ready for review!

ghost · 2021-04-16T19:32:41Z

/hold cancel

tekton-robot · 2021-04-16T19:33:53Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	71.0%	76.3%	5.3
pkg/apis/pipeline/v1beta1/version_validation.go	Do not exist	100.0%
test/gate.go	Do not exist	0.0%

tekton-robot · 2021-04-16T19:38:36Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	71.0%	76.3%	5.3
pkg/apis/pipeline/v1beta1/version_validation.go	Do not exist	100.0%
test/gate.go	Do not exist	0.0%

vdemeester

/lgtm

pritidesai · 2021-04-19T22:08:36Z

pkg/apis/config/feature_flags.go

+ case AlphaAPIFields, StableAPIFields:
+ *feature = value
+ default:
+ *feature = defaultValue


I don't have a strong opinion here (I like the way its implemented) but we are ignoring any value other than stable and alpha and defaulting to stable which is different from bool feature flags. I think we should at least log this somewhere that its being defaulted to stable. On the other side, we could be ignoring a spelling mistake when someone tried to set it to alpha and by default we are operating stable APIs.

Right, I think it makes sense to at least log something in case of the value not being one we support, but the user will have to check the logs of the controller or the webhook (and know what to look for) to see it. But agreed we should log it anyway.

Indeed, I didn't want our machinery to end up in a broken state because the user mis-typed a value in the configmap. But adding a log line is a great idea; I'll do that.

Hm. We don't have access to ctx in a lot of places that leverage this code. This might be a bigger change than I thought.

@pritidesai @vdemeester would you be ok with me using the go log package to print this message? The reason is that NewFeatureFlagsFromConfigMap is passed into knative and called by the framework without a ctx variable. I can't think of a way to shim support for the zap logger into that function without completely rewriting how the config package is structured. Since that function then calls NewFeatureFlagsFromMap, which in turn calls setEnabledAPIFields (from this PR), I don't see a great way to handle this except for leaning on the go log package.

Any other ideas on ways we could tackle this?

From the owners meeting just now:

We could return an error in a similar fashion as we do when boolean flags aren't correctly set to "true" / "false"

Knative pkg may be able to help us out here - they may have some configmap validation webhook tooling we can leverage to validate these on submission

I've gone ahead and implemented (1) - returning an error if the value is mis-spelled. Created #3895 to investigate validating the configmap contents somehow.

pritidesai · 2021-04-20T21:42:11Z

pkg/apis/config/testdata/feature-flags-invalid-boolean.yaml

+ name: feature-flags
+ namespace: tekton-pipelines
+data:
+ "disable-creds-init": "im-really-not-a-valid-boolean"


pritidesai · 2021-04-20T21:56:00Z

pkg/apis/pipeline/v1beta1/version_validation.go

+// ValidateEnabledAPIFields checks that the enable-api-fields feature gate is set
+// to the wantVersion value and, if not, returns an error stating which feature
+// is dependent on the version and what the current version actually is.
+func ValidateEnabledAPIFields(ctx context.Context, featureName, wantVersion string) *apis.FieldError {


@sbwsg I am trying to understand the usage of this function, its only referenced in the unit tests for now. How do we envision this being utilized in future?

Ah, good catch, there's an example of usage over in the Step & Sidecar Workspaces PR: https:/tektoncd/pipeline/pull/3700/files#diff-988b00fa4f188a96ff5901c0bbc9f65d4a8355417185d4fe0ae97d8c14e04417R131-R133

The below snippet ensures that the new field is only valid to use when enable-api-fields is alpha:

for stepIdx, step := range steps { if len(step.Workspaces) != 0 { errs = errs.Also(ValidateEnabledAPIFields(ctx, "step workspaces", config.AlphaAPIFields).ViaIndex(stepIdx).ViaField("steps")) }

pritidesai · 2021-04-20T22:45:40Z

test/path_filtering.go

+// if the enable-api-fields feature flag is currently set
+// to "alpha" then all "stable" and "beta" examples would be
+// excluded. Similarly when the flag is set to "stable",
+// all "alpha" and "beta" examples are filtered out.


I am trying to understand this, please bear with me.

If I am reading this right, the feature flag value is considered as absolute and only allows examples of that particular version to be tested.

But based on the TEP, setting the feature gate to alpha means the controller must support alpha and stable both:

Feature Versions -> beta alpha

stable x

alpha x x

x == "enabled"

Should we run stable examples with alpha feature gate? Am I mixing things up here? 🙃 Is this meant to work with something else? 🤔

I think you're absolutely right here. I've updated the func to run both alpha and stable examples when enable-api-fields is "alpha".

tekton-robot · 2021-04-21T20:42:04Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	71.0%	82.5%	11.5
pkg/apis/pipeline/v1beta1/version_validation.go	Do not exist	100.0%
test/gate.go	Do not exist	0.0%

tekton-robot · 2021-04-21T21:01:27Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	71.0%	82.5%	11.5
pkg/apis/pipeline/v1beta1/version_validation.go	Do not exist	100.0%
test/gate.go	Do not exist	0.0%

tekton-robot · 2021-04-21T21:07:04Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	71.0%	82.5%	11.5
pkg/apis/pipeline/v1beta1/version_validation.go	Do not exist	100.0%
test/gate.go	Do not exist	0.0%

pritidesai · 2021-04-22T17:56:40Z

test/path_filtering.go

+ output = bytes.TrimSpace(output)
+ output = bytes.Trim(output, "\"")
+ if len(output) == 0 {
+ enableAPIFields = "stable"


I think we are missing return here. The enableAPIFields will be overwritten by the next statement even if output is empty.

continue... instead of returning "stable"

Thank you. I need test code for my test code >.< This should be fixed now, I've changed it to:

if len(output) == 0 { output = []byte("stable") }

Implements TEP-0033: https:/tektoncd/community/blob/main/teps/0033-tekton-feature-gates.md This commit introduces a new feature flag, "enable-api-fields", that can be set to "alpha" or "stable". The default is "stable". This commit provides a test helper, `requireGate` that allows an integration test to be easily skipped if "enable-api-fields" is not "alpha". This commit also provides a way to run examples specific to each feature-gate: New examples can be added under a directory called "alpha". When the "enable-api-fields" flag is set to "alpha" the test runner will _only_ run those tests in "alpha" directories. Similarly if the flag is "stable" then the examples under "alpha" will be prevented from running. See the Step and Sidecar Workspaces branch for examples that use this new mechanism: - https:/sbwsg/pipeline/commit/ba4ca8d18e5a0c6a3ffc3e71424b95385314a5cf#diff-b33f1e031894c58e1c3f02fd3291bc14bdae76333d9c19a3b672b88736821a6b - https:/sbwsg/pipeline/commit/ba4ca8d18e5a0c6a3ffc3e71424b95385314a5cf#diff-71bc2dcaacd303b560c738c3f1fb2e4eb35ebe628e8cf53038f7b85c63ccfb5d - https:/sbwsg/pipeline/commit/ba4ca8d18e5a0c6a3ffc3e71424b95385314a5cf#diff-568a92e9e9f73030ce0dfbb15adf76f4e65e7e28aef0474421ba5b415911f9c7

tekton-robot · 2021-04-22T18:50:21Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	71.0%	82.5%	11.5
pkg/apis/pipeline/v1beta1/version_validation.go	Do not exist	100.0%
test/gate.go	Do not exist	0.0%

pritidesai · 2021-04-22T20:30:03Z

thanks a bunch @sbwsg for all your hard work in implementing this, great work 👍

/approve

waveywaves · 2021-04-27T16:30:29Z

This looks great @sbwsg 😄
Can't wait to use it to move #3857 behind a feature gate !
Also, are there any guidelines around when/how a feature 'graduates' from alpha to stable ? These guidelines might be a little bit obvious with respect to testing rigour and community acceptability, but it would be nice to have these covered in a doc somewhere probably.

waveywaves · 2021-04-27T16:31:22Z

/approve

tekton-robot · 2021-04-27T16:31:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pritidesai, waveywaves

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:

~~OWNERS~~ [pritidesai]

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

ghost · 2021-04-27T16:36:27Z

lgtm 🙏

ghost · 2021-04-27T16:44:54Z

Also, are there any guidelines around when/how a feature 'graduates' from alpha to stable ? These guidelines might be a little bit obvious with respect to testing rigour and community acceptability, but it would be nice to have these covered in a doc somewhere probably.

Great question! At the moment the TEP leaves it to the project OWNERS discretion:

This will likely require the feature being available for at least one release if not several releases (and maybe use in dogfooding) to be able to collect and handle user feedback.

ghost · 2021-04-28T15:14:20Z

@waveywaves would you be able to switch your approve to an lgtm? This would put the PR into a mergeable state since Priti already added the approve. Thanks a lot for reviewing!

waveywaves · 2021-04-28T15:38:55Z

I dunno if it will work, but I'll give it a go.

/lgtm

afrittoli · 2021-04-28T14:09:35Z

pkg/apis/config/feature_flags.go

 return &tc, nil
 }

+// setEnabledAPIFields sets the "enable-api-fields" flag based on the content of a given map.
+// If the feature gate is invalid or missing then the flag is set to its default.


The comment seems out of sync with the code.
If the feature gate is invalid then an error is returned.

afrittoli · 2021-04-28T14:10:35Z

pkg/apis/config/feature_flags_test.go

@@ -97,10 +100,27 @@ func TestGetFeatureFlagsConfigName(t *testing.T) {
 }
 }

+func TestNewFeatureFlagsConfigMapErrors(t *testing.T) {


afrittoli · 2021-04-28T14:21:46Z

test/path_filtering.go

+
+func getFeatureGate() (string, error) {
+ if enableAPIFields == "" {
+ cmd := exec.Command("kubectl", "get", "configmap", "feature-flags", "-n", "tekton-pipelines", "-o", `jsonpath="{.data['enable-api-fields']}"`)


NIT: For testing purposes I guess we can assume the namespace will indeed be tekton-pipelines, but it might be nice to have this configurable.

afrittoli · 2021-04-28T16:11:54Z

test/path_filtering.go

@@ -0,0 +1,87 @@
+// +build examples


We don't have unit tests for this module

In #3881 we added the new "enable-api-fields" feature gate and some related helpers. There were a couple of comments I missed before the PR got merged so adding them here instead. This commit adds tests for the bits of "test/path_filtering.go" that can be checked without a cluster running Tekton Pipelines.

tektoncd/pipeline#3881 is the "main" PR for this, but it's pretty clear that there are other aspects of feature gates floating around not yet completed, hence going with `implementing` here. Signed-off-by: Andrew Bayer <[email protected]>

tektoncd/pipeline#3881 is the "main" PR for this Signed-off-by: Andrew Bayer <[email protected]>

ghost requested review from vdemeester, bobcatfish and afrittoli April 14, 2021 16:26

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Apr 14, 2021

tekton-robot requested a review from dlorenc April 14, 2021 16:26

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 14, 2021

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 14, 2021

ghost mentioned this pull request Apr 14, 2021

Add Step and Sidecar Workspaces feature #3700

Merged

4 tasks

vdemeester reviewed Apr 16, 2021

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 16, 2021

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 16, 2021

vdemeester reviewed Apr 19, 2021

View reviewed changes

tekton-robot assigned vdemeester Apr 19, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2021

pritidesai reviewed Apr 19, 2021

View reviewed changes

ghost added this to the Pipelines 0.24 milestone Apr 20, 2021

pritidesai reviewed Apr 20, 2021

View reviewed changes

pritidesai reviewed Apr 22, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2021

ghost mentioned this pull request Apr 23, 2021

Add enable-api-fields: alpha to pipeline's CI runs #3896

Closed

souleb mentioned this pull request Apr 23, 2021

Add a Timeouts optional field to pipelinerun #3843

Merged

4 tasks

tekton-robot assigned waveywaves Apr 28, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 28, 2021

tekton-robot merged commit 94a6095 into tektoncd:main Apr 28, 2021

afrittoli reviewed Apr 28, 2021

View reviewed changes

ghost mentioned this pull request Apr 28, 2021

Last minute fixups from review feedback on "enable-api-fields" PR #3904

Merged

5 tasks

abayer mentioned this pull request Dec 15, 2021

TEP-0033: Mark as implemented tektoncd/community#584

Merged

abayer added a commit to abayer/community that referenced this pull request Dec 16, 2021

TEP-0033: Mark as implemented

dd68cdd

tektoncd/pipeline#3881 is the "main" PR for this Signed-off-by: Andrew Bayer <[email protected]>

tekton-robot pushed a commit to tektoncd/community that referenced this pull request Dec 16, 2021

TEP-0033: Mark as implemented

93ad960

tektoncd/pipeline#3881 is the "main" PR for this Signed-off-by: Andrew Bayer <[email protected]>

Implement TEP-0033: Add enable-api-fields feature-flag #3881

Implement TEP-0033: Add enable-api-fields feature-flag #3881

Conversation

ghost commented Apr 14, 2021 • edited by ghost Loading

Changes

Submitter Checklist

Release Notes

ghost commented Apr 14, 2021

tekton-robot commented Apr 14, 2021

tekton-robot commented Apr 14, 2021

tekton-robot commented Apr 14, 2021

tekton-robot commented Apr 14, 2021

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 16, 2021 • edited by ghost Loading

ghost commented Apr 16, 2021

tekton-robot commented Apr 16, 2021

ghost commented Apr 16, 2021

tekton-robot commented Apr 16, 2021

ghost commented Apr 16, 2021

ghost commented Apr 16, 2021

tekton-robot commented Apr 16, 2021

tekton-robot commented Apr 16, 2021

vdemeester left a 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

ghost Apr 20, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Apr 20, 2021 • edited by ghost 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

Choose a reason for hiding this comment

tekton-robot commented Apr 21, 2021

tekton-robot commented Apr 21, 2021

tekton-robot commented Apr 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Apr 22, 2021

pritidesai commented Apr 22, 2021

waveywaves commented Apr 27, 2021 • edited Loading

waveywaves commented Apr 27, 2021

tekton-robot commented Apr 27, 2021

ghost commented Apr 27, 2021 • edited by ghost Loading

ghost commented Apr 27, 2021

ghost commented Apr 28, 2021

waveywaves commented Apr 28, 2021

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

ghost commented Apr 14, 2021 •

edited by ghost

Loading

ghost commented Apr 16, 2021 •

edited by ghost

Loading

ghost Apr 20, 2021 •

edited by ghost

Loading

ghost Apr 20, 2021 •

edited by ghost

Loading

waveywaves commented Apr 27, 2021 •

edited

Loading

ghost commented Apr 27, 2021 •

edited by ghost

Loading