Add resolution types to the API #4502

ghost · 2022-01-20T20:29:16Z

Changes

TEP-0060 introduces remote resource resolution to Tekton Pipelines, allowing PipelineRuns and TaskRuns to reference Pipelines and Tasks in remote places like git repos.

This PR adds the fields for the remote resolution feature to the v1beta1.TaskRef and v1beta1.PipelineRef types. Also includes validations and unit test coverage. These fields can only be included when the enable-api-fields feature-flag is set to alpha. Even then, utilizing these fields only results in the reconcilers printing errors.

Submitter Checklist

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

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

Syntax support for remote resolution, an alpha feature in development, has been added to pipelineRef and taskRef fields. Using the remote resolution fields is not yet possible, however, and will only result in errors.

tekton-robot · 2022-01-20T20:31:19Z

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/pipeline/v1beta1/pipelineref_validation.go	Do not exist	96.9%
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	98.5%	98.3%	-0.2
pkg/apis/pipeline/v1beta1/taskref_validation.go	Do not exist	96.9%

tekton-robot · 2022-01-20T21:02:19Z

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/pipeline/v1beta1/pipelineref_validation.go	Do not exist	96.9%
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	98.5%	98.3%	-0.2
pkg/apis/pipeline/v1beta1/taskref_validation.go	Do not exist	96.9%

ghost · 2022-01-20T21:07:31Z

/kind feature

tekton-robot · 2022-01-20T21:17:41Z

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/pipeline/v1beta1/pipelineref_validation.go	Do not exist	96.8%
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	98.5%	98.3%	-0.2
pkg/apis/pipeline/v1beta1/taskref_validation.go	Do not exist	96.8%

lbernick · 2022-02-02T22:02:51Z

pkg/apis/pipeline/v1beta1/taskrun_validation_test.go

@@ -261,6 +261,63 @@ func TestTaskRunSpec_Invalidate(t *testing.T) {
 		},
 		wantErr: apis.ErrInvalidValue("breakito is not a valid breakpoint. Available valid breakpoints include [onFailure]", "debug.breakpoint"),
 		wc:      enableAlphaAPIFields,
+	}, {


could you add some test cases with valid taskrefs for the new syntax?

Good catch, updated with tests of valid resolution syntax when alpha gate is enabled.

lbernick · 2022-02-02T22:03:50Z

pkg/apis/pipeline/v1beta1/swagger.json

@@ -1471,6 +1471,24 @@
        }
      }
    },
+    "v1beta1.ResolverRef": {


Just for my own understanding what is this file for? Are the changes generated or manual?

It's generated by, I think, ./hack/openapigen.sh

I believe swagger.json can be used to generate API clients for other programming languages. I don't know of any examples of folks using it, though.

lbernick · 2022-02-02T22:10:21Z

pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go

@@ -370,9 +370,10 @@ func TestPipelineRun_Validate(t *testing.T) {

 func TestPipelineRunSpec_Invalidate(t *testing.T) {


similar comment as for TaskRef, could you add a test case where the ref is valid?

Added a test for taskref with resolver field + alpha gate and a test for taskref with resolver field + resource fields + alpha gate.

bobcatfish · 2022-02-03T17:53:57Z

Love the incremental PR approach @sbwsg 💃 🎉 !!

tekton-robot · 2022-02-07T13:25:36Z

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/pipeline/v1beta1/pipelineref_validation.go	Do not exist	96.8%
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	98.8%	98.6%	-0.1
pkg/apis/pipeline/v1beta1/taskref_validation.go	Do not exist	96.8%

lbernick · 2022-02-07T14:11:31Z

/lgtm

jerop

Thank you @sbwsg for the iterative approach in implementing Remote Resolution 😀

Left some comments about the types ResolverRef:

Resolver - considering using string alias instead of string (K8s convention)
Resource - considering using Parameters instead of map (K8s convention)

What do you think about adding the relevant documentation for the API change and validation so that we can review it alongside the change? (we can add a bolded disclaimer that the functionality doesn't work yet, even though users can pass in the fields)

jerop · 2022-02-10T00:21:59Z

pkg/apis/pipeline/v1beta1/resolver_types.go

+	// Resource contains the parameters used to identify the
+	// referenced Tekton resource. Example entries might include
+	// "repo" or "path" but the set of params ultimately depends on
+	// the chosen resolver.
+	// +optional
+	Resource map[string]string `json:"resource,omitempty"`


I'm wondering about usingmap[string]string for Resource - could we use Parameters instead?
Especially considering that the proposal suggests using object Parameters in this field when it's available - maybe using a list of string Parameters here would help in the transition later?

Note that the fields under resource: are validated by a resolver, not by Tekton Pipelines. TEP-0075 (Dictionary
Params) describes adding support for JSON object schema to Params. It would make sense to bring in the same schema support for the resource: field as well and eventually resolvers might be able to publish the schema they accept so that Pipelines can enforce it during validation of a TaskRun or PipelineRun.
~ TEP-0060

K8s API convention encouraging use of sub-objects instead of maps: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#lists-of-named-subobjects-preferred-over-maps

Thanks for the feedback. I've changed this to use the following syntax:

taskRef: remote: git resource: - name: repo value: https://github.com/tektoncd/catalog.git - name: path value: /task/golang-build/0.3/golang-build.yaml

jerop · 2022-02-10T00:38:52Z

pkg/apis/pipeline/v1beta1/resolver_types.go

+	// Resolver is the name of the resolver that should perform
+	// resolution of the referenced Tekton resource, such as "git".
+	// +optional
+	Resolver string `json:"resolver,omitempty"`


The TEP proposes a specific set of resolvers: git, bundle and in-cluster

What do you think about using aliases for the strings, instead of strings directly, and adding validation for resolvers themselves?

K8s API convention encouraging use of string aliases when fields will have a list of allowed values (enumerations): https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#constants

This field is intentionally free-form. The resolution feature is intended to allow integration with any resolver an operator wants to install and that could include ones that Tekton hasn't explicitly named in its source code. e.g. "svn", "bzr", "gcs"

So, I guess I don't see this being in the spirit of what the Resolution TEP is aiming to achieve for the Tekton project but I also don't want to introduce freeform data if you feel strongly that you'd prefer to have the options explicitly named. Let me know.

Thanks for the additional context!

How about we declare the Resolver type, specify the ones we provide, then integrators can specify their own:

# in tekton pipelines type Resolver string const ( git Resolver = "git" bundle Resolver = "bundle" cluster Resolver = "cluster" )

# in an integration const ( svn tekton.Resolver = "svn" bzr tekton.Resolver = "bzr" gcs tekton.Resolver = "gcs" )

Similarly to how we use selection.Operator from K8s apimachinery in operator field in when expressions

What do you think?

OK, I've updated the type from string to v1beta1.ResolverName.

jerop · 2022-02-10T00:50:37Z

/assign

tekton-robot · 2022-02-10T11:49:36Z

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/pipeline/v1beta1/pipelineref_validation.go	Do not exist	96.8%
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	98.8%	98.6%	-0.1
pkg/apis/pipeline/v1beta1/taskref_validation.go	Do not exist	96.8%

tekton-robot · 2022-02-10T11:50:21Z

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/pipeline/v1beta1/pipelineref_validation.go	Do not exist	96.8%
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	98.8%	98.6%	-0.1
pkg/apis/pipeline/v1beta1/taskref_validation.go	Do not exist	96.8%

tekton-robot · 2022-02-10T12:13:39Z

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/pipeline/v1beta1/pipelineref_validation.go	Do not exist	96.8%
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	98.8%	98.6%	-0.1
pkg/apis/pipeline/v1beta1/taskref_validation.go	Do not exist	96.8%

ghost · 2022-02-10T12:20:58Z

I've added initial documentation to taskruns.md, pipelinerun.md, and install.md

tekton-robot · 2022-02-10T12:25:36Z

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/pipeline/v1beta1/pipelineref_validation.go	Do not exist	96.8%
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	98.8%	98.6%	-0.1
pkg/apis/pipeline/v1beta1/taskref_validation.go	Do not exist	96.8%

tekton-robot · 2022-02-10T16:25:08Z

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/pipeline/v1beta1/pipelineref_validation.go	Do not exist	96.8%
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	98.8%	98.6%	-0.1
pkg/apis/pipeline/v1beta1/taskref_validation.go	Do not exist	96.8%

TEP-0060 adds remote resource resolution to Tekton Pipelines, allowing PipelineRuns and TaskRuns to reference Pipelines and Tasks in remote places like git repos. This PR adds the initial alpha syntax for the remote resolution feature to the v1beta1.TaskRef and v1beta1.PipelineRef types along with validation for them. Some initial documentation is added describing the syntax for remote tasks and pipelines.

tekton-robot · 2022-02-10T16:48:35Z

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/pipeline/v1beta1/pipelineref_validation.go	Do not exist	96.8%
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	98.8%	98.6%	-0.1
pkg/apis/pipeline/v1beta1/taskref_validation.go	Do not exist	96.8%

jerop

excited to try this out soon, thank you @sbwsg 🎉

tekton-robot · 2022-02-10T17:11:45Z

[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:

~~OWNERS~~ [jerop]

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

jerop · 2022-02-10T17:16:13Z

Reopened the issue tracking the flakey test: TestTaskRunPipelineRunCancel

/test pull-tekton-pipeline-alpha-integration-tests

lbernick · 2022-02-10T19:11:36Z

/lgtm
🎉

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 20, 2022

tekton-robot requested review from jerop and vdemeester January 20, 2022 20:29

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 20, 2022

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 20, 2022

lbernick reviewed Feb 2, 2022

View reviewed changes

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 7, 2022

tekton-robot assigned lbernick Feb 7, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2022

jerop reviewed Feb 10, 2022

View reviewed changes

tekton-robot assigned jerop Feb 10, 2022

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 10, 2022

jerop approved these changes Feb 10, 2022

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 10, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 10, 2022

tekton-robot merged commit 5d3f23e into tektoncd:main Feb 10, 2022

lbernick mentioned this pull request Mar 21, 2022

WIP: [TEP-0100] Embedded TaskRun and Run statuses in PipelineRuns #4694

Closed

5 tasks

abayer mentioned this pull request Jul 28, 2022

Add types and client for Resolution #5200

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add resolution types to the API #4502

Add resolution types to the API #4502

ghost commented Jan 20, 2022

tekton-robot commented Jan 20, 2022

tekton-robot commented Jan 20, 2022

ghost commented Jan 20, 2022

tekton-robot commented Jan 20, 2022

lbernick Feb 2, 2022

ghost Feb 7, 2022

lbernick Feb 2, 2022

ghost Feb 7, 2022

lbernick Feb 2, 2022 •

edited

Loading

ghost Feb 7, 2022

bobcatfish commented Feb 3, 2022

tekton-robot commented Feb 7, 2022

lbernick commented Feb 7, 2022

jerop left a comment

jerop Feb 10, 2022

ghost Feb 10, 2022

jerop Feb 10, 2022

ghost Feb 10, 2022

jerop Feb 10, 2022

ghost Feb 10, 2022

jerop commented Feb 10, 2022

tekton-robot commented Feb 10, 2022

tekton-robot commented Feb 10, 2022

tekton-robot commented Feb 10, 2022

ghost commented Feb 10, 2022

tekton-robot commented Feb 10, 2022

tekton-robot commented Feb 10, 2022

tekton-robot commented Feb 10, 2022

jerop left a comment

tekton-robot commented Feb 10, 2022

jerop commented Feb 10, 2022

lbernick commented Feb 10, 2022

		@@ -370,9 +370,10 @@ func TestPipelineRun_Validate(t *testing.T) {

		func TestPipelineRunSpec_Invalidate(t *testing.T) {

Add resolution types to the API #4502

Add resolution types to the API #4502

Conversation

ghost commented Jan 20, 2022

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jan 20, 2022

tekton-robot commented Jan 20, 2022

ghost commented Jan 20, 2022

tekton-robot commented Jan 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbernick Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish commented Feb 3, 2022

tekton-robot commented Feb 7, 2022

lbernick commented Feb 7, 2022

jerop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerop commented Feb 10, 2022

tekton-robot commented Feb 10, 2022

tekton-robot commented Feb 10, 2022

tekton-robot commented Feb 10, 2022

ghost commented Feb 10, 2022

tekton-robot commented Feb 10, 2022

tekton-robot commented Feb 10, 2022

tekton-robot commented Feb 10, 2022

jerop left a comment

Choose a reason for hiding this comment

tekton-robot commented Feb 10, 2022

jerop commented Feb 10, 2022

lbernick commented Feb 10, 2022

lbernick Feb 2, 2022 •

edited

Loading