pipeline icon indicating copy to clipboard operation
pipeline copied to clipboard

Published 20 hours ago • verified publisher icon tektoncd

Add "beta" value to `enable-api-fields`

Open lbernick opened this issue 3 years ago • 11 comments

Changes

This commit adds a "beta" option to the "enable-api-fields" feature flag. No functional changes.

/kind misc

Submitter Checklist

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

  • [x] Has Docs included if any changes are user facing
  • [x] Has Tests included if any functionality added or changed
  • [x] Follows the commit message standard
  • [x] Meets the Tekton contributor standards (including functionality, content, code)
  • [x] 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)
  • 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

lbernick avatar Aug 15 '22 18:08 lbernick

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 81.8% 78.9% -2.9
pkg/apis/pipeline/v1/task_validation.go 98.0% 98.0% 0.0

tekton-robot avatar Aug 15 '22 18:08 tekton-robot

LGTM Thank you Lee.

JeromeJu avatar Aug 22 '22 14:08 JeromeJu

The more we use the enable-api-fields feature flag, the more confusing it is getting for me. Aside from the all-or-nothing behavior slight problem, my feeling is that we are abusing the feature-flag for a lot of things. Shouldn't we make v1 with the field we only want to support instead ? There is going to be some time when we will (have to) support v1 and v1beta1 and thus users will still be able to use step.resources from v1beta1 object. Having this "it's in the API (so documented in openapi "possibly", …) but only when the beta feature-flag is enable is very confusing to me. It's even more confusing to me as we aim to get read of it, but as an end-user, I am mostly assuming what would be under the beta feature-flag is on the way to be "promoted".

TBH I agree, I would also assume that anything in beta is on its way to be promoted rather than deprecated. Maybe we should reopen discussion around maintaining multiple api versions rather than the enable-api-fields feature flag? @dibyom

For this PR, would it be helpful to have it just add the beta flag and not use it for any fields right now? (especially since compute resources are still under discussion) Or does it not make sense to add dead code?

lbernick avatar Aug 24 '22 14:08 lbernick

TBH I agree, I would also assume that anything in beta is on its way to be promoted rather than deprecated. Maybe we should reopen discussion around maintaining multiple api versions rather than the enable-api-fields feature flag? @dibyom

I am not sure I understand the "reopen discussion around maintaining multiple api versions". We are/will maintain multiple api versions, this is a given. For a given period of time (according to API policies, …), we will support and serve v1beta1 as well as v1. Which one is stored, and how the controller does its magic is an implementation detail, but from a user perspective, we will support both version, we have to. There is no way around that.

For this PR, would it be helpful to have it just add the beta flag and not use it for any fields right now? (especially since compute resources are still under discussion) Or does it not make sense to add dead code?

It kinds of makes it dead code, but I think it's fine though, as it provides the "framework" for beta fields to be added / promoted later one.

vdemeester avatar Aug 24 '22 15:08 vdemeester

I am not sure I understand the "reopen discussion around maintaining multiple api versions". We are/will maintain multiple api versions, this is a given.

Sorry, I should have been more clear-- I meant instead of adding features gated behind an alpha feature flag to a beta CRD, creating an alpha CRD and adding features there, and later on adding them to the beta CRD. This discussion is probably out of scope for this PR though.

It kinds of makes it dead code, but I think it's fine though, as it provides the "framework" for beta fields to be added / promoted later one.

Sounds good, I'll make those changes.

lbernick avatar Aug 24 '22 15:08 lbernick

Sorry, I should have been more clear-- I meant instead of adding features gated behind an alpha feature flag to a beta CRD, creating an alpha CRD and adding features there, and later on adding them to the beta CRD. This discussion is probably out of scope for this PR though.

Right, now I understand it a bit more 🙃. Indeed this is a bit out of the scope of this PR, and it comes with its "set" of challenges.

vdemeester avatar Aug 24 '22 15:08 vdemeester

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 81.8% 78.9% -2.9

tekton-robot avatar Aug 24 '22 16:08 tekton-robot

@dibyom mentioned wanting to discuss potential updates to TEP 33 (whether there is value in a "beta" feature flag at all as opposed to alpha and stable) before merging this PR, so I've added it to the next api wg agenda and I think we can put it on hold for now. FYI @vdemeester I think he might have chatted with you about this as well.

/hold

lbernick avatar Sep 06 '22 15:09 lbernick

@dibyom mentioned wanting to discuss potential updates to TEP 33 (whether there is value in a "beta" feature flag at all as opposed to alpha and stable) before merging this PR, so I've added it to the next api wg agenda and I think we can put it on hold for now. FYI @vdemeester I think he might have chatted with you about this as well.

/hold

Yes indeed. I was about to put a hold on this one for that same reason 👼🏼

vdemeester avatar Sep 06 '22 15:09 vdemeester

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 81.8% 78.9% -2.9

tekton-robot avatar Sep 22 '22 14:09 tekton-robot

/hold cancel

discussed at api wg-- don't want to block moving some existing features to beta on the larger discussion about better api versioning, so I'm reopening this pr

lbernick avatar Sep 26 '22 16:09 lbernick

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop

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

tekton-robot avatar Sep 26 '22 18:09 tekton-robot

Thanks for the feedback @jerop, done!

lbernick avatar Sep 26 '22 18:09 lbernick

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 81.8% 78.9% -2.9

tekton-robot avatar Sep 26 '22 18:09 tekton-robot

/lgtm

abayer avatar Sep 26 '22 19:09 abayer