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

Enable Bundles and Custom Tasks when feature gate is set to "alpha" #3908

Merged
merged 1 commit into from May 3, 2021
Merged

Enable Bundles and Custom Tasks when feature gate is set to "alpha" #3908

merged 1 commit into from May 3, 2021

Conversation

ghost
Copy link

@ghost ghost commented Apr 29, 2021

Changes

Prior to this commit the Tekton Bundles and Custom Task features were only enabled
when their individual flags were set to "true" in the feature-flags configmap.

After this commit the two features are also enabled when enable-api-fields is "alpha".

Note that it is not possible to set the "alpha" feature gate but disable Bundles / Custom Tasks. These features are "alpha features" so they're always enabled regardless of their own feature flags if the gate is set to "alpha".

Submitter Checklist

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

  • Docs included if any changes are user facing
  • Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including
    functionality, content, code)
  • Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

Switching the "enable-api-fields" feature gate to "alpha" also turns on Tekton Bundles and Custom Tasks support, since these are alpha features.

@ghost ghost added the kind/misc Categorizes issue or PR as a miscellaneuous one. label Apr 29, 2021
@ghost ghost added this to the Pipelines 0.24 milestone Apr 29, 2021
@ghost ghost requested a review from afrittoli April 29, 2021 20:03
@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Apr 29, 2021
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 29, 2021
@tekton-robot
Copy link
Collaborator

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 82.5% 83.7% 1.2

@tekton-robot
Copy link
Collaborator

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 82.5% 83.7% 1.2

@tekton-robot
Copy link
Collaborator

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 82.5% 83.7% 1.2

@tekton-robot
Copy link
Collaborator

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 82.5% 83.7% 1.2

@ghost ghost changed the title Enable Bundles and Custom Tasks when alpha feature gate is set to "alpha" Enable Bundles and Custom Tasks when feature gate is set to "alpha" Apr 29, 2021
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.

Thank you for this - it looks really nice!
Just a couple of minor comments :)
/approve

docs/install.md Outdated
Features currently in "alpha" are:

- [Tekton Bundles](./taskruns.md#tekton-bundles)
- Custom Tasks
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Ahhhh this is exactly what I needed, thanks!

pkg/apis/config/feature_flags_test.go Show resolved Hide resolved
// test if none of the feature-flags in the given map match
// what's in the feature-flags ConfigMap. It will fatally fail
// the test if it cannot get the feature-flag configmap.
func requireAnyGate(gates map[string]string) func(context.Context, *testing.T, *clients, string) {
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2021
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

/meow

@tekton-robot
Copy link
Collaborator

@vdemeester: cat image

In response to this:

/meow

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.

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, 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:
  • OWNERS [afrittoli,vdemeester]

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

@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 30, 2021
@ghost
Copy link
Author

ghost commented Apr 30, 2021

Thanks for the reviews! Updated with a test for "stable" gate + bundles:"true" + custom-tasks: "true" & links from the install doc to runs.md.

Edit: stealthy ninja pushed an update to developer docs too so that requireAnyGate is documented.

@tekton-robot
Copy link
Collaborator

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 82.5% 83.7% 1.2

@ghost
Copy link
Author

ghost commented Apr 30, 2021

Also just manually verified that custom tasks tests do indeed run when either the gate is "alpha" or the feature's flag is "true". Here's what a skip looks like:

=== RUN   TestCustomTask
    init_test.go:132: Create namespace arendelle-5d8z7 to deploy to
    init_test.go:148: Verify SA "default" is created in namespace "arendelle-5d8z7"
    gate.go:32: No feature flag matching "enable-custom-tasks": "true" or "enable-api-fields": "alpha"
--- SKIP: TestCustomTask (1.04s)

@tekton-robot
Copy link
Collaborator

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 82.5% 83.7% 1.2

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.

Thank you!
/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2021
@afrittoli
Copy link
Member

/test pull-tekton-pipeline-integration-tests

1 similar comment
@ghost
Copy link
Author

ghost commented Apr 30, 2021

/test pull-tekton-pipeline-integration-tests

…pha"

Prior to this commit the Tekton Bundles and Custom Task features were only enabled
when their individual flags were set to "true" in the feature-flags configmap.

After this commit the two features are enabled if enable-api-fields is "alpha" or
if their individual flags are set to "true". Note that it is _not_ possible to
set the "alpha" feature gate but disable Bundles / Custom Tasks. These features
are "alpha features" so they're always enabled regardless of their own feature
flags if the gate is set to "alpha".
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2021
@tekton-robot
Copy link
Collaborator

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 82.5% 83.7% 1.2

@ghost
Copy link
Author

ghost commented Apr 30, 2021

The workspace-in-sidecar example was failing over and over again. Adding explicit resources.requests to the example seems to have fixed that 🤞 : https://github.com/tektoncd/pipeline/pull/3908/files#diff-d3a6963f568541460e24acddf2bcc5d3337bbbc8632e91a63ee59c6084891a2e

This is ready for a final lgtm (hopefully!)

@pritidesai
Copy link
Member

I tried running custom task which failed with an error:

{"level":"error","ts":"2021-05-03T18:37:17.747Z","logger":"tekton-pipelines-controller","caller":"pipelinerun/pipelinerun.go:244","msg":"Reconcile error: Couldn't retrieve Task \"testloop\": tasks.tekton.dev \"testloop\" not found","commit":"13a40db","knative.dev/controller":"git.luolix.top.tektoncd.pipeline.pkg.reconciler.pipelinerun.Reconciler","knative.dev/kind":"tekton.dev.PipelineRun","knative.dev/traceid":"13fb3555-5186-4077-be81-573413801e63","knative.dev/key":"default/testpipeline-run-854c7","stacktrace":"github.com/tektoncd/pipeline/pkg/reconciler/pipelinerun.(*Reconciler).ReconcileKind\n\tgit.luolix.top/tektoncd/pipeline/pkg/reconciler/pipelinerun/pipelinerun.go:244\ngit.luolix.top/tektoncd/pipeline/pkg/client/injection/reconciler/pipeline/v1beta1/pipelinerun.(*reconcilerImpl).Reconcile\n\tgit.luolix.top/tektoncd/pipeline/pkg/client/injection/reconciler/pipeline/v1beta1/pipelinerun/reconciler.go:240\nknative.dev/pkg/controller.(*Impl).processNextWorkItem\n\tknative.dev/pkg@v0.0.0-20210331065221-952fdd90dbb0/controller/controller.go:530\nknative.dev/pkg/controller.(*Impl).RunContext.func3\n\tknative.dev/pkg@v0.0.0-20210331065221-952fdd90dbb0/controller/controller.go:468"}

But testloop is not a resource of type tasks.tekton.dev:

tkn pipelinerun logs testpipeline-run-854c7 -f -n default
Pipeline default/testpipeline can't be Run; it contains Tasks that don't exist: Couldn't retrieve Task "testloop": tasks.tekton.dev "testloop" not found

I am troubleshooting this 🙃

@pritidesai
Copy link
Member

oh, nevermind, my bad 😭 I didn't enable alpha APIs 🤣

1 similar comment
@pritidesai
Copy link
Member

oh, nevermind, my bad 😭 I didn't enable alpha APIs 🤣

@pritidesai
Copy link
Member

looks great, thanks @sbwsg
/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 3, 2021
@pritidesai
Copy link
Member

Enabling the alpha APIs sets the appropriate feature flags:

  EnableTektonOCIBundles: (bool) true,
  EnableCustomTasks: (bool) true,
  EnableAPIFields: (string) (len=5) "alpha"

@ghost
Copy link
Author

ghost commented May 3, 2021

Awesome, thanks for taking the time to review and try it out so thoroughly @pritidesai !

@tekton-robot tekton-robot merged commit 565c55b into tektoncd:main May 3, 2021
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/misc Categorizes issue or PR as a miscellaneuous one. 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.

4 participants