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.

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

Merged
merged 2 commits into from
Oct 26, 2023

Conversation

JeromeJu
Copy link
Member

@JeromeJu JeromeJu commented Sep 5, 2023

Changes

This PR contains 2 commits which:

  • refactors the feature-versioning developer doc
  • adds the per-feature flag struct and documentations for
    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:

  • 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
  • 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.
  • Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Please use per feature flags for new API-driven feature gating.

@tekton-robot
Copy link
Collaborator

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

@tekton-robot tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesnt merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. labels Sep 5, 2023
@tekton-robot
Copy link
Collaborator

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 91.7% 92.5% 0.9

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 7, 2023
@JeromeJu JeromeJu mentioned this pull request Oct 2, 2023
7 tasks
@JeromeJu JeromeJu force-pushed the tep0138-per-feature-flag branch from 24c0ea9 to c31ca1e Compare October 10, 2023 21:12
@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 10, 2023
@tekton-robot
Copy link
Collaborator

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 91.7% 88.8% -2.9

@JeromeJu JeromeJu force-pushed the tep0138-per-feature-flag branch 2 times, most recently from 8e05369 to 2cc7606 Compare October 13, 2023 16:48
@JeromeJu JeromeJu marked this pull request as ready for review October 13, 2023 17:59
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 13, 2023
@JeromeJu JeromeJu force-pushed the tep0138-per-feature-flag branch from 2cc7606 to 138a661 Compare October 16, 2023 15:10
@chitrangpatel
Copy link
Contributor

There seems to be only one commit but the PR description says two. May be it needs to be updated.

@QuanZhang-William
Copy link
Member

/assign

@JeromeJu
Copy link
Member Author

JeromeJu commented Oct 16, 2023

There seems to be only one commit but the PR description says two. May be it needs to be updated.

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

@chitrangpatel
Copy link
Contributor

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.

@afrittoli
Copy link
Member

/assign

@JeromeJu
Copy link
Member Author

JeromeJu commented Oct 16, 2023

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.

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.

@tekton-robot
Copy link
Collaborator

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
cmd/entrypoint/runner.go 86.2% 84.5% -1.7
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 99.2% 98.3% -0.8

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2023
@JeromeJu JeromeJu force-pushed the tep0138-per-feature-flag branch from 2c51424 to 984d88e Compare October 25, 2023 13:19
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 25, 2023
@tekton-robot tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesnt merit a release note. labels Oct 25, 2023
@JeromeJu
Copy link
Member Author

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? 🙏 🙏 🙏
cc @tektoncd/core-maintainers

@QuanZhang-William
Copy link
Member

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
@JeromeJu JeromeJu force-pushed the tep0138-per-feature-flag branch 2 times, most recently from 773983e to feab572 Compare October 25, 2023 21:07
@tekton-robot
Copy link
Collaborator

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Member

@afrittoli afrittoli left a 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 Show resolved Hide resolved
| 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`~~ |
Copy link
Member

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

Copy link
Member Author

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)

Copy link
Member Author

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.

api_compatibility_policy.md Outdated Show resolved Hide resolved
docs/developers/feature-versioning.md Outdated Show resolved Hide resolved
docs/developers/feature-versioning.md Outdated Show resolved Hide resolved
docs/developers/feature-versioning.md Outdated Show resolved Hide resolved
@JeromeJu JeromeJu force-pushed the tep0138-per-feature-flag branch from feab572 to 4bd4613 Compare October 26, 2023 14:23
This commit adds the per-feature flag struct and documentations for
new features in TektonCD pipeline.

part of: TEP-0138
@JeromeJu JeromeJu force-pushed the tep0138-per-feature-flag branch from 4bd4613 to 3b37c44 Compare October 26, 2023 14:44
@JeromeJu JeromeJu requested a review from afrittoli October 26, 2023 14:56
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2023
@tekton-robot tekton-robot merged commit fe42e6b into tektoncd:main Oct 26, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants