Do not require enable-api-fields=alpha for spire #6939

lbernick · 2023-07-18T15:09:09Z

Prior to this commit, enabling spire support also required setting "enable-api-fields" to alpha. "enforce-nonfalsifiability" is the only behavioral feature flag that also requires setting "enable-api-fields" to alpha, even though it is not the only behavioral feature flag in an alpha state.

This commit removes the requirement that cluster operators set enable-api-fields to alpha in order to use spire.
In addition to creating a consistent approach among behavioral feature flags, this will allow cluster operators to opt into spire without having to opt into all alpha API fields.

Note: Spire support is not yet functional.

/kind misc

Submitter Checklist

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

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
n/a Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
n/a Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

tekton-robot · 2023-07-18T15:17:44Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	92.6%	92.3%	-0.3

JeromeJu

Thanks @lbernick !

pkg/apis/config/testdata/feature-flags-spire-with-stable.yaml

JeromeJu · 2023-07-18T15:24:46Z

docs/spire.md

@@ -64,7 +64,7 @@ When a TaskRun is created:
 ## Enabling TaskRun result attestations

 To enable TaskRun attestations:
-1. Make sure `enforce-nonfalsifiability` is set to `"spire"` and `enable-api-fields` is set to `"alpha"` in the `feature-flags` configmap, see [`install.md`](./install.md#customizing-the-pipelines-controller-behavior) for details
+1. Make sure `enforce-nonfalsifiability` is set to `"spire"`. See [`additional-configs.md`](./additional-configs.md#customizing-the-pipelines-controller-behavior) for details


NIT: It looks like that the enforce-nonfalsifiablity is neither set in additonal-configs/customizing-the-pipelines-controller-behavior section, is it WIP? 🤔

yes, spire is still non-functional

afrittoli

Thanks @lbernick - Spire is not controlled by the user via API - it should never have been an API flag, only a behavioural one.

/lgtm

afrittoli · 2023-07-18T21:32:17Z

/retest

tekton-robot · 2023-07-18T21:37:57Z

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	92.6%	92.3%	-0.3

Prior to this commit, enabling spire support also required setting "enable-api-fields" to alpha. "enforce-nonfalsifiability" is the only behavioral feature flag that also requires setting "enable-api-fields" to alpha, even though it is not the only behavioral feature flag in an alpha state. This commit removes the requirement that cluster operators set enable-api-fields to alpha in order to use spire. In addition to creating a consistent approach among behavioral feature flags, this will allow cluster operators to opt into spire without having to opt into all alpha API fields. Note: Spire support is not yet functional.

tekton-robot · 2023-07-18T22:19:12Z

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	92.6%	92.3%	-0.3

tekton-robot · 2023-07-18T22:20:56Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	92.6%	92.3%	-0.3

JeromeJu · 2023-07-18T22:26:21Z

/lgtm

chuangw6 · 2023-07-19T14:00:55Z

LGTM.

cc @jerop for visibility since https://github.com/tektoncd/pipeline/pull/6527/files added requirements of setting "enable-api-fields" to

afrittoli

/approve

tekton-robot · 2023-07-19T16:13:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, JeromeJu

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~~ [afrittoli]

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

jerop · 2023-07-19T16:45:14Z

/hold

jerop · 2023-07-19T16:46:54Z

This change was introduced in https://github.com/tektoncd/pipeline/pull/5902/files#r1086050834 following a discussion between @bobcatfish and @jagathprakash so it'd be great to hear from them -- this change sgtm

@chuangw6 https://github.com/tektoncd/pipeline/pull/6527/files was only refactoring the implementation, it didn't make any functional changes

lbernick · 2023-07-20T13:56:36Z

Thanks for the additional context! @bobcatfish and @jagathprakash aren't actively working on OSS tekton anymore, and @chuangw6 is taking over spire work and has given a LGTM. I'll reach out to the two of them, but I think it makes the most sense to evaluate this change based on the reasons given in the commit message. It looks like the linked conversation was more about returning an error instead of silently failing when enable-api-fields was not set to alpha, rather than the decision to couple the two flags in the first place.

vdemeester · 2023-07-20T14:14:25Z

/lgtm

jagathprakash · 2023-07-20T16:26:56Z

/lgtm

tekton-robot · 2023-07-20T16:26:58Z

@jagathprakash: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

jerop · 2023-07-20T16:30:34Z

/unhold

thanks for reviewing @jagathprakash 🙏🏾

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/misc Categorizes issue or PR as a miscellaneuous one. labels Jul 18, 2023

tekton-robot requested review from dibyom and pritidesai July 18, 2023 15:09

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 18, 2023

lbernick assigned chitrangpatel, chuangw6 and JeromeJu Jul 18, 2023

JeromeJu approved these changes Jul 18, 2023

View reviewed changes

afrittoli reviewed Jul 18, 2023

View reviewed changes

tekton-robot assigned afrittoli Jul 18, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2023

lbernick force-pushed the spire branch from b5c0f5d to b5b7610 Compare July 18, 2023 22:12

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2023

afrittoli reviewed Jul 19, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 19, 2023

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 19, 2023

tekton-robot assigned vdemeester Jul 20, 2023

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 20, 2023

tekton-robot merged commit f5b1a3e into tektoncd:main Jul 20, 2023

JeromeJu mentioned this pull request Jul 24, 2023

Epic: Improve feature versioning #6966

Closed

13 tasks

jerop mentioned this pull request Oct 3, 2023

TEP-0127: Larger Results via Sidecar Logs [Feedback Requested] #6133

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not require enable-api-fields=alpha for spire #6939

Do not require enable-api-fields=alpha for spire #6939

lbernick commented Jul 18, 2023

tekton-robot commented Jul 18, 2023

JeromeJu left a comment

JeromeJu Jul 18, 2023

lbernick Jul 18, 2023

afrittoli left a comment

afrittoli commented Jul 18, 2023

tekton-robot commented Jul 18, 2023

tekton-robot commented Jul 18, 2023

tekton-robot commented Jul 18, 2023

JeromeJu commented Jul 18, 2023

chuangw6 commented Jul 19, 2023

afrittoli left a comment

tekton-robot commented Jul 19, 2023

jerop commented Jul 19, 2023

jerop commented Jul 19, 2023

lbernick commented Jul 20, 2023

vdemeester commented Jul 20, 2023

jagathprakash commented Jul 20, 2023

tekton-robot commented Jul 20, 2023

jerop commented Jul 20, 2023

Do not require enable-api-fields=alpha for spire #6939

Do not require enable-api-fields=alpha for spire #6939

Conversation

lbernick commented Jul 18, 2023

Submitter Checklist

Release Notes

tekton-robot commented Jul 18, 2023

JeromeJu left a comment

Choose a reason for hiding this comment

JeromeJu Jul 18, 2023

Choose a reason for hiding this comment

lbernick Jul 18, 2023

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli commented Jul 18, 2023

tekton-robot commented Jul 18, 2023

tekton-robot commented Jul 18, 2023

tekton-robot commented Jul 18, 2023

JeromeJu commented Jul 18, 2023

chuangw6 commented Jul 19, 2023

afrittoli left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 19, 2023

jerop commented Jul 19, 2023

jerop commented Jul 19, 2023

lbernick commented Jul 20, 2023

vdemeester commented Jul 20, 2023

jagathprakash commented Jul 20, 2023

tekton-robot commented Jul 20, 2023

jerop commented Jul 20, 2023