-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[TEP0138] Add Per-feature Flag Struct for New Features #7090
[TEP0138] Add Per-feature Flag Struct for New Features #7090
Conversation
Skipping CI for Draft Pull Request. |
The following is the coverage report on the affected files.
|
24c0ea9
to
c31ca1e
Compare
The following is the coverage report on the affected files.
|
8e05369
to
2cc7606
Compare
2cc7606
to
138a661
Compare
There seems to be only one commit but the PR description says two. May be it needs to be updated. |
/assign |
Thanks @chitrangpatel . Updated! Also to be on the note for this change: I previously thought that we could just provide an example since there were no upcoming new features yet. But now there are a couple that I've realized: i.e. #7162 and CEL expression that had been mentioned in TEP process. Happy to pair on/ walk through the process if a new feature is getting in pipeline! cc @tektoncd/core-collaborators |
Do you think we can have an example of how we introduce a feature "enable-foo" in developer docs? In there we can at least provide an example of that we need to do to the struct and how it can be used internally in our codebase. Finally, when we add a new feature, we might want to also enable it in https://github.com/tektoncd/pipeline/blob/main/test/e2e-tests-kind-prow-alpha.env I think a document showing this journey will be useful when we start adding new features under per-feature flags. |
/assign |
138a661
to
a685fe8
Compare
Thanks @chitrangpatel , it makes sense to add it as an example. And I totally agree developers dir is the right place to go 🙏 Working on it as a 2nd commit rn. |
The following is the coverage report on the affected files.
|
2c51424
to
984d88e
Compare
Since #6941 has been merged and this is unblocked, while we have a handful of new features(i.e. #7277, #7255) that might come in. Could we prioritize merging this PR as the more new feature flags come into the repo the more future "patching" of the usage with the new struct there are going to be? 🙏 🙏 🙏 |
Thanks for the PR, generally looks good to me. Added some small comments. |
This commit refactors the api-versioning.md developer doc which previously mixed API versioning and feature versioning. The API-driven feature versioning was mingled with the process of adding a new CRD apiVersion. To have better clarification of the difference between API versioning and the feature versioning, a new Markdown file feature-versioning is introduced in light of TEP-0138. /kind documentation part of tektoncd#6965
773983e
to
feab572
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: QuanZhang-William, Yongxuanzhang 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @JeromeJu - I left some comments.
api_compatibility_policy.md
Outdated
| After v0.53.0 | [Existing features](#snapshot-of-existent-beta-and-alpha-api-driven-features-as-of-v0530httpsgithubcomtektoncdpipelinereleasestagv0530) till v0.53.0 | `enable-api-fields` | | ||
| After v0.53.0 | New features after v0.53.0 | Per-feature flags | | ||
| ... | | | | ||
| Future release | All [existing features](#snapshot-of-existent-beta-and-alpha-api-driven-features-as-of-v0530httpsgithubcomtektoncdpipelinereleasestagv0530) stabilized or deprecated | Sunset ~~`enable-api-fields`~~ | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"All existing features" will sound confusing in future, despite the link.
I find the table a bit difficult to parse, what about this?
Releases | Global flag enable-api-fields |
Per-feature flags |
---|---|---|
Prior to v0.53.0 | All API features | Alpha/beta non-API features only |
After v0.53.0 | Alpha/beta features in v0.53.0 | Alpha/beta non-API features and API features introduced after v0.53.0 |
All alpha/beta features in v0.53.0 become stable or are removed | Sunset enable-api-fields |
All alpha/beta API and NON-API features |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense! I made some minor changes to it because behavioural flags will retain their behaviour. (I also added a note below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also updated the links at tektoncd/community#1091.
feab572
to
4bd4613
Compare
This commit adds the per-feature flag struct and documentations for new features in TektonCD pipeline. part of: TEP-0138
4bd4613
to
3b37c44
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Changes
This PR contains 2 commits which:
new features in TektonCD pipeline.
part of: TEP-0138
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes