[TEP-0133]: Configure Default Resolver #6317

QuanZhang-William · 2023-03-08T17:43:02Z

Changes

This commit introduces a new default-resolver-type field to the config-defaults ConfigMap, which configures the default resolver type to be used when the resolver is not explicitly provided in the input. Supporting the default resolver type improves simplicity at the authoring time. More details can be found in TEP-0113: Configure Default Resolver.

/kind feature

Submitter Checklist

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

Has Docs included if any changes are user facing
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)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

[TEP-0133] Add "default-resolver-type" field in the "default-configs" ConfigMap to configure default resolver

tekton-robot · 2023-03-08T17:43:04Z

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

QuanZhang-William · 2023-03-08T17:45:36Z

/test all

tekton-robot · 2023-03-08T17:50:55Z

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/default.go	90.0%	88.5%	-1.5
pkg/apis/pipeline/v1beta1/taskrun_defaults.go	100.0%	95.8%	-4.2

tekton-robot · 2023-03-08T17:52:46Z

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/default.go	90.0%	88.5%	-1.5
pkg/apis/pipeline/v1beta1/taskrun_defaults.go	100.0%	95.8%	-4.2

tekton-robot · 2023-03-08T18:35:13Z

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/default.go	90.0%	88.5%	-1.5

tekton-robot · 2023-03-08T18:35:51Z

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/default.go	90.0%	88.5%	-1.5

QuanZhang-William · 2023-03-08T18:55:46Z

Tracking issue: #5907

chuangw6 · 2023-03-08T19:00:28Z

/assign

lbernick

thanks @QuanZhang-William! Is there also validation for taskrefs with a default resolver configured?

docs/resolution.md

pkg/apis/pipeline/v1/pipeline_defaults_test.go

QuanZhang-William · 2023-03-08T23:52:30Z

thanks @QuanZhang-William! Is there also validation for taskrefs with a default resolver configured?

This is a good question and it is a bit tricky. The short answer is no, and I feel it is not necessary?:

If the default resolver is NOT configured, we already have validations and test cases cover the empty resolver scenario.
If the default resolver is configured, the value will be populated at Mutation Webhook phase which happens before the validation, So the validation will never receive a request which empty resolver in this case (which I think is the scenario you talked about to validation).

lbernick · 2023-03-09T14:42:46Z

thanks @QuanZhang-William! Is there also validation for taskrefs with a default resolver configured?

This is a good question and it is a bit tricky. The short answer is no, and I feel it is not necessary?:

If the default resolver is NOT configured, we already have validations and test cases cover the empty resolver scenario.

If the default resolver is configured, the value will be populated at Mutation Webhook phase which happens before the validation, So the validation will never receive a request which empty resolver in this case (which I think is the scenario you talked about to validation).

ah that's a good point-- I wish there was a better way to unit test this, but this seems fine.

just a quick comment, can you add the TEP in the release note?

tekton-robot · 2023-03-13T12:44:54Z

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/default.go	90.0%	88.5%	-1.5
pkg/apis/pipeline/v1beta1/pipelinerun_defaults.go	92.3%	93.3%	1.0

tekton-robot · 2023-03-13T12:46:01Z

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/default.go	90.0%	88.5%	-1.5
pkg/apis/pipeline/v1beta1/pipelinerun_defaults.go	92.3%	93.3%	1.0

QuanZhang-William · 2023-03-13T13:06:21Z

pkg/reconciler/pipelinerun/pipelinespec/pipelinespec_test.go

+				Spec: v1beta1.PipelineRunSpec{
+					PipelineRef: &v1beta1.PipelineRef{
+						ResolverRef: v1beta1.ResolverRef{
+							Resolver: "foo",


The PipelineRun resolver must have been pre-populated at this point

QuanZhang-William · 2023-03-13T13:11:22Z

Thanks @QuanZhang-William . It seems like that setting default resolver for PipelineRef in a PipelineRun is missing.

With that, we should also consider the special case that the pipeline spec referred by PipelineRef also references other remote tasks. Example:
PipelineRun
...
spec:
   pipelineRef:
       resolver: git # if missing, the default resolver can be set by the mutating webhook
       params:
            ....
...
Pipeline referenced by the above PipelineRun
...
spec:
     tasks:
        - name: clone-source
          taskRef:
               resolver: bundles # how to set default resolver for this?
               params:
                   ...
Setting default resolver for PipelineRef can be done in mutating webhook, but how do we set default resolver for its inner TaskRef used by the remote pipeline? It seems like it's already handled by the reconciler by recalling pipelineSpec.SetDefaults(ctx) after fetching the remote pipeline spec.

If this worked as intended, I think it's worth adding test cases for it and mentioning this special case in the TEP. wdyt?

Thanks @chuangw6! Good catch on the missing PipelineRef change! I have added it and PTAL.

Regarding the edge case, yes, this is intended and I have added a test case for it. I think the examples given in the tep has elaborated this idea.

Yongxuanzhang · 2023-03-13T21:43:10Z

I wonder if we're missing unit tests in pkg/apis/config/default_test.go ? It reads config from file and compare if it is expected.
Other code lgtm

This commit introduces a new `default-resolver-type` field to the `config-defaults` ConfigMap, which configures the default resolver type to be used when the `resolver` is not explicitly provided in the input. Supporting the default resolver type improves simplicity at the authoring time. More details can be found in [TEP-0113: Conifgure Default Resolver]. /kind feature [TEP-0113: Conifgure Default Resolver]: https://github.com/tektoncd/community/blob/main/teps/0133-configure-default-resolver.md

QuanZhang-William · 2023-03-14T14:56:05Z

I wonder if we're missing unit tests in pkg/apis/config/default_test.go ? It reads config from file and compare if it is expected. Other code lgtm

Thanks @Yongxuanzhang, just added the tests, PTAL!

tekton-robot · 2023-03-14T15:02:01Z

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/default.go	90.0%	90.4%	0.4
pkg/apis/pipeline/v1beta1/pipelinerun_defaults.go	92.3%	93.3%	1.0

tekton-robot · 2023-03-14T15:03:00Z

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/default.go	90.0%	90.4%	0.4
pkg/apis/pipeline/v1beta1/pipelinerun_defaults.go	92.3%	93.3%	1.0

Yongxuanzhang · 2023-03-14T17:13:12Z

Thank you! Test coverage also improved!
/lgtm

dibyom · 2023-03-14T17:37:14Z

/approve
/hold

(Adding a hold in case another approver wants to review this PR but I think this is good to go)

tekton-robot · 2023-03-14T17:37:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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~~ [dibyom]

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

dibyom · 2023-03-15T17:43:16Z

/cc @pritidesai

pritidesai · 2023-03-15T18:07:56Z

(FYI: I am working with a limited internet/cellular connectivity in the Bay Area)

how does this impact existing tasks and pipelines? The default is set to ‘git’ but how about params? How does this add value when I have to specify params for git? Please note that I am not objecting to this change, but trying to understand the impact of this.

QuanZhang-William · 2023-03-15T18:17:57Z

(FYI: I am working with a limited internet/cellular connectivity in the Bay Area)

how does this impact existing tasks and pipelines? The default is set to ‘git’ but how about params? How does this add value when I have to specify params for git? Please note that I am not objecting to this change, but trying to understand the impact of this.

Hi @pritidesai, there is no impact to existing Task and Pipeline as all the resources today have the resolver field specified explicitly.

Simplicity is the main benefit of this feature. This feature can be used together with the default parameters we support for resolvers, for example:

by configuring the default values like

  # Setting this flag to a resolver type as the default resolver
  default-resolver-type: "git"
  
  # The below fields are git resolver specific and have been supported already
  # The git url to fetch the remote resource from when using anonymous cloning.
  default-url: "https://github.com/tektoncd/catalog.git"
  # The git revision to fetch the remote resource from with either anonymous cloning or the authenticated API.
  default-revision: "main"

the author can reduce a task run from

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
 name: demo-task-run
spec:
 taskRef:
   resolver: git
   params:
   - name: url
     value: https://github.com/tektoncd/catalog.git
   - name: revision
     value: main
   - name: pathInRepo
     value: task/golang-build/0.1/golang-build.yaml
...

to

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
 name: demo-task-run
spec:
 taskRef:
  params:
  - name: pathInRepo
    value: task/golang-build/0.1/golang-build.yaml
...

The use cases are discuss in TEP-0133

dibyom · 2023-03-16T02:20:49Z

/hold cancel

Chatted with Priti earlier in the day, I think we can merge once it has been rebased

tekton-robot added kind/feature Categorizes issue or PR as related to a new feature. release-note-none Denotes a PR that doesnt merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 8, 2023

tekton-robot requested review from dibyom and vdemeester March 8, 2023 17:43

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 8, 2023

tekton-robot added 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 Mar 8, 2023

QuanZhang-William force-pushed the config-resolver-as-default branch from 9ba0c42 to ed4ebbd Compare March 8, 2023 18:27

QuanZhang-William marked this pull request as ready for review March 8, 2023 18:28

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 Mar 8, 2023

tekton-robot requested review from afrittoli and wlynch March 8, 2023 18:29

QuanZhang-William changed the title ~~[TEP-0133]: Configure Default resolver~~ [TEP-0133]: Configure Default Resolver Mar 8, 2023

jerop added this to the Pipelines v0.46 milestone Mar 8, 2023

tekton-robot assigned chuangw6 Mar 8, 2023

lbernick reviewed Mar 8, 2023

View reviewed changes

docs/resolution.md Outdated Show resolved Hide resolved

pkg/apis/pipeline/v1/pipeline_defaults_test.go Outdated Show resolved Hide resolved

QuanZhang-William force-pushed the config-resolver-as-default branch from ed4ebbd to e782f2a Compare March 9, 2023 15:55

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 Mar 9, 2023

QuanZhang-William force-pushed the config-resolver-as-default branch from 4df79e6 to 96c5f78 Compare March 13, 2023 12:39

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 13, 2023

QuanZhang-William commented Mar 13, 2023

View reviewed changes

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 13, 2023

QuanZhang-William force-pushed the config-resolver-as-default branch from 96c5f78 to f037d71 Compare March 14, 2023 14:55

tekton-robot assigned Yongxuanzhang Mar 14, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 14, 2023

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 14, 2023

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2023

tekton-robot requested a review from pritidesai March 15, 2023 17:43

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 16, 2023

tekton-robot merged commit 9ff4f88 into tektoncd:main Mar 16, 2023

QuanZhang-William mentioned this pull request Mar 21, 2023

Allow cluster operator to configure default resolver #5907

Closed

jerop mentioned this pull request Mar 28, 2023

Default Resolver is not set for Finally Tasks #6449

Closed

QuanZhang-William mentioned this pull request Nov 2, 2023

[TEP-0142] Add ResolverRef to Ref #7322

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEP-0133]: Configure Default Resolver #6317

[TEP-0133]: Configure Default Resolver #6317

QuanZhang-William commented Mar 8, 2023 •

edited

Loading

tekton-robot commented Mar 8, 2023

QuanZhang-William commented Mar 8, 2023

tekton-robot commented Mar 8, 2023

tekton-robot commented Mar 8, 2023

tekton-robot commented Mar 8, 2023

tekton-robot commented Mar 8, 2023

QuanZhang-William commented Mar 8, 2023

chuangw6 commented Mar 8, 2023

lbernick left a comment

QuanZhang-William commented Mar 8, 2023 •

edited

Loading

lbernick commented Mar 9, 2023

tekton-robot commented Mar 13, 2023

tekton-robot commented Mar 13, 2023

QuanZhang-William Mar 13, 2023

QuanZhang-William commented Mar 13, 2023

Yongxuanzhang commented Mar 13, 2023

QuanZhang-William commented Mar 14, 2023

tekton-robot commented Mar 14, 2023

tekton-robot commented Mar 14, 2023

Yongxuanzhang commented Mar 14, 2023

dibyom commented Mar 14, 2023

tekton-robot commented Mar 14, 2023

dibyom commented Mar 15, 2023

pritidesai commented Mar 15, 2023

QuanZhang-William commented Mar 15, 2023

dibyom commented Mar 16, 2023

[TEP-0133]: Configure Default Resolver #6317

[TEP-0133]: Configure Default Resolver #6317

Conversation

QuanZhang-William commented Mar 8, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Mar 8, 2023

QuanZhang-William commented Mar 8, 2023

tekton-robot commented Mar 8, 2023

tekton-robot commented Mar 8, 2023

tekton-robot commented Mar 8, 2023

tekton-robot commented Mar 8, 2023

QuanZhang-William commented Mar 8, 2023

chuangw6 commented Mar 8, 2023

lbernick left a comment

Choose a reason for hiding this comment

QuanZhang-William commented Mar 8, 2023 • edited Loading

lbernick commented Mar 9, 2023

tekton-robot commented Mar 13, 2023

tekton-robot commented Mar 13, 2023

QuanZhang-William Mar 13, 2023

Choose a reason for hiding this comment

QuanZhang-William commented Mar 13, 2023

Yongxuanzhang commented Mar 13, 2023

QuanZhang-William commented Mar 14, 2023

tekton-robot commented Mar 14, 2023

tekton-robot commented Mar 14, 2023

Yongxuanzhang commented Mar 14, 2023

dibyom commented Mar 14, 2023

tekton-robot commented Mar 14, 2023

dibyom commented Mar 15, 2023

pritidesai commented Mar 15, 2023

QuanZhang-William commented Mar 15, 2023

dibyom commented Mar 16, 2023

QuanZhang-William commented Mar 8, 2023 •

edited

Loading

QuanZhang-William commented Mar 8, 2023 •

edited

Loading