[TEP-0138]: Decouple API and Feature Versioning Proposal #1034
Conversation
tekton-robot
added
the
do-not-merge/work-in-progress
tekton-robot
added
the
size/M
JeromeJu
changed the title
tekton-robot
added
the
kind/tep
JeromeJu
changed the title
tekton-robot
removed
the
do-not-merge/work-in-progress
JeromeJu
force-pushed
the
per-feature-flag
branch
2 times, most recently
from
9f9add6
to
56009ac
Compare
|
/assign @vdemeester @afrittoli |
|
/assign |
JeromeJu
force-pushed
the
per-feature-flag
branch
from
56009ac
to
666aac6
Compare
JeromeJu
force-pushed
the
per-feature-flag
branch
3 times, most recently
from
4b7a6cd
to
5eeacf5
Compare
| ```go | ||
| type FeatureFlag struct { | ||
| // Name of the feature flag | ||
| Name string |
There was a problem hiding this comment.
NIT: What is the name used for? Perhaps for logging purposes?
Perhaps we could do something similar to what we have for behavioural flags:
type APIFeatureFlags struct {
Foo FeatureFlag
Bar FeatureFlag
}
There was a problem hiding this comment.
I agree the name field could be kept for logging purposes.
As for the grouping of all the API feature flags, my understanding is that such struct would make sense if we would like to turn on behavioural features of a certain stability level. But I am not sure if this is going to be necessary while we fully switch to per-feature flags (i.e. after all current beta/alpha features stabilized)?
There was a problem hiding this comment.
Thank you for the updates.
I think the current proposal looks really good - the only comment I have is about the deprecation / locking of feature flags.
Thanks for you for the detailed testing section. I think it's good to have some numbers about how many tests we may run. Beyond that we will probably have to decide on a feature by feature basis, based on the likelihood for features to collide and use cases of features used together.
JeromeJu
force-pushed
the
per-feature-flag
branch
2 times, most recently
from
2abd749
to
3740308
Compare
|
Thanks @pritidesai , I also do appreciate your right on-the-spot suggestions. In terms of the feature request, I updated it in the future work section. Also created tektoncd/pipeline#7093 for further discussion. I think this might also be a current feature request even without per feature flag to be introduced 🤔 |
Thanks @afrittoli for the review! I understand the cases that we need to test out where two features could be used together but not to have their feature flags correlated. Otherwise they shall be separated? (I think there are two flags related at the moment but one is deprecated) |
tekton-robot
added
the
needs-rebase
JeromeJu
force-pushed
the
per-feature-flag
branch
from
3740308
to
7c8be45
Compare
tekton-robot
removed
the
needs-rebase
JeromeJu
force-pushed
the
per-feature-flag
branch
2 times, most recently
from
4b41175
to
a5462c5
Compare
|
|
||
| - **Task and Pipeline Authors:** Tekton Pipeline and Task authors will get to know the list of features that are turned on from their service providers. See [future work](#future-work) for the better communication from cluster operators to users for more details. | ||
|
|
||
| ### Sunset `enable-api-fields` after existing features stabilize |
There was a problem hiding this comment.
It could be worth mentioning the plans for new features being added before the per-feature flag model is implemented.
Some of the feature proposal/implementation:
There was a problem hiding this comment.
I agree that we shall give heads up for existing features. But I am not sure if keeping it in the current TEP would be helpful since basically all new features coming in after this TEP being merged shall use per-feature flag model ideally.(Plus merging this TEP sooner so to avoid such situations) Maybe giving heads up in the WG could be a nice option WDYT?
There was a problem hiding this comment.
Do you mean that new features coming in should follow per-feature flag after this TEP is merged or implemented?
There was a problem hiding this comment.
Yes:) And it looks like most maintainers are on board with this TEP, we are hoping to get this merged by EOD. I will go ahead and update the API-compatibility policy ASAP.
| - The same `pipeline` and `task` definitions might not be applied to a cluster with `enable-api-fields` set to `stable` after this change is implemented. | ||
| - The `pipeline` and `task` definitions in `v1beta1` implementing [beta features](https://github.com/tektoncd/pipeline/blob/main/docs/additional-configs.md#beta-features) will not be applied to a cluster with `enable-api-fields` set to `stable` Such pipeline with beta features used in `v1beta1` apiVersion would not result in a successful pipelineRun because of [the validation after the conversion to `v1` as the storage version](https://github.com/tektoncd/pipeline/blob/d9d2d1760fa534e2dfb16ca656cdf2d293a5900e/pkg/apis/pipeline/`v1`/taskref_validation.go#L39). Since in this example, resolver is still a beta feature, this pipeline should not have been applied to the cluster that has not set `enable-api-fields=beta` in the first place. | ||
|
|
||
| ### Per feature flag for _new_ api-driven features |
There was a problem hiding this comment.
It seems like the API versions and feature stability levels are already decoupled with the Change existing validation to decouple feature and API versioning proposal. I'm wondering what is the motivation to use per-feature flag model?
There was a problem hiding this comment.
Thanks for the comment. I've put back the missing pieces:)
The reason that this was not previously included was that we first aimed to decouple feature versioning only and while moving forward with the discussion in API WG we've decided to use per-feature flag as the solution to decouple versioning in order to avoid the potential issues from setting the default stability level in the long run.
This commit adds the proposals for decoupling api and feature versioning. This TEP is marked as 'implementable'.
JeromeJu
force-pushed
the
per-feature-flag
branch
from
a5462c5
to
433ce63
Compare
|
Thanks for the reviews again @QuanZhang-William and @afrittoli . I've updated the TEP according to the feedback and comments along with more thorough formatting. Due to the new features that are coming into pipeline, for example the feature flag usage in new features i.e. CEL expression and onError field, it might be helpful and to have less confusion to get this merged sooner than other features come in. Much appreciated if we could get another look and approvals 🙏 @afrittoli @vdemeester |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dibyom, pritidesai, vdemeester 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 |
|
/lgtm |
| | Beta | Disabled | | ||
| | Alpha | Disabled | | ||
|
|
||
| Note that per-feature flags that have stabilized cannot be disabled. We will deprecate the per-feature flag after it has become stable and then remove it eventually after the deprecation period according to the API compatibility policy. We will give deprecation and later removel notice of the per-feature flags via release notes after their promotion to `stable`. Cluster operators who do not want such opt-in features would have enough notice to implement admission controllers on their own to disable the feature. |
There was a problem hiding this comment.
@JeromeJu sorry, I thought I commented on this before, but I must have missed it 🙏
Keeping the feature flag around, deprecated but unusable does not make much sense to me.
Making it unusable provides the same behaviour as removing it, the deprecation has no effect really.
The only reason I could see to keep the flag around would be if we started validating the content of the config file (which we do not do today) - but even so, I think failing config validation would be preferrable to silently ignoring a feature flag.
There was a problem hiding this comment.
Never mind, I think I misread this section 🙏
A small update to the table might help - but feel free to ignore this - I can also propose a PR with the update if that's ok for you.
| Feature stability level | Default |
| ----------------------- | -------- |
| Stable | Enabled; Cannot be disabled once the flag is removed (after deprecation) |
| Beta | Disabled |
| Alpha | Disabled |
There was a problem hiding this comment.
Thanks for the suggestion, PTAL #1091 🙏
This commit adds the proposals for decoupling api and feature versioning. This TEP is marked as 'implementable'.