Move remote resolution out of alpha #5515

abayer · 2022-09-19T14:50:03Z

Changes

Closes #4710

There are two major aspects to this change:

Creating a v1beta1 for ResolutionRequest, changing from parameters as a map[string]string to params (field name rename for openapi-gen API rule validation reasons) as a []pipelinev1beta1.Param. This is being done to match ResolverRef.Params, which is a []pipelinev1beta1.Param. We originally left ResolutionRequest.Parameters as map[string]string since it already was that in v1alpha1 and changing it would require ResolutionRequest v1beta1, hence doing this now. Changing to []pipelinev1beta1.Param is needed to support array and object params for resolvers, and we're going with a slice rather than a map because that's how you're supposed to do this in Kubernetes. =)
Promoting the remote resolution functionality from alpha to beta, meaning on by default for v1beta1 Pipelines etc, and requiring enable-api-fields: beta for v1 Pipelines etc. Additionally, we're undoing the questionable decision to split the resolvers out into resolvers.yaml, separate from release.yaml, and enabling the built-in resolvers.

Deprecation of the legacy taskRef.bundle and pipelineRef.bundle syntax will be handled in a separate PR (#5514).

/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

action required: Any resolvers being used other than built-in resolvers will need to be updated to use ResolutionRequest v1beta1. Remote resolution of Pipelines and Tasks promoted to beta, and ResolutionRequest v1beta1 introduced to support array and object parameters for resolvers.

abayer · 2022-09-19T14:50:10Z

/wip

tekton-robot · 2022-09-19T14:55:06Z

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/reconciler/pipelinerun/resources/pipelineref.go	89.7%	88.9%	-0.9
pkg/reconciler/taskrun/resources/taskref.go	85.7%	84.9%	-0.8
pkg/resolution/resolver/bundle/params.go	75.9%	78.1%	2.3
pkg/resolution/resolver/cluster/resolver.go	84.7%	86.1%	1.4
pkg/resolution/resolver/framework/fakeresolver.go	90.0%	91.7%	1.7
pkg/resolution/resolver/git/resolver.go	84.6%	84.9%	0.3
pkg/resolution/resolver/hub/resolver.go	70.6%	73.7%	3.1

tekton-robot · 2022-09-19T15:14:52Z

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/reconciler/pipelinerun/resources/pipelineref.go	89.7%	88.9%	-0.9
pkg/reconciler/taskrun/resources/taskref.go	85.7%	84.9%	-0.8
pkg/resolution/resolver/bundle/params.go	75.9%	78.1%	2.3
pkg/resolution/resolver/cluster/resolver.go	84.7%	86.1%	1.4
pkg/resolution/resolver/framework/fakeresolver.go	90.0%	91.7%	1.7
pkg/resolution/resolver/git/resolver.go	84.6%	84.9%	0.3
pkg/resolution/resolver/hub/resolver.go	70.6%	73.7%	3.1

abayer · 2022-09-19T15:24:41Z

/test check-pr-has-kind-label

tekton-robot · 2022-09-19T15:24:42Z

@abayer: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

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

The following commands are available to trigger optional jobs:

/test pull-tekton-pipeline-go-coverage

Use /test all to run all jobs.

In response to this:

/test check-pr-has-kind-label

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 · 2022-09-19T15:49:51Z

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/reconciler/pipelinerun/resources/pipelineref.go	89.7%	88.9%	-0.9
pkg/reconciler/taskrun/resources/taskref.go	85.7%	84.9%	-0.8
pkg/resolution/resolver/bundle/params.go	75.9%	78.1%	2.3
pkg/resolution/resolver/cluster/resolver.go	84.7%	86.1%	1.4
pkg/resolution/resolver/framework/fakeresolver.go	90.0%	91.7%	1.7
pkg/resolution/resolver/git/resolver.go	84.6%	84.9%	0.3
pkg/resolution/resolver/hub/resolver.go	70.6%	73.7%	3.1

tekton-robot · 2022-09-19T16:33:50Z

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/v1/pipeline_types.go	95.7%	95.5%	-0.2
pkg/apis/pipeline/v1beta1/pipeline_types.go	96.8%	96.7%	-0.1
pkg/reconciler/pipelinerun/resources/pipelineref.go	89.7%	88.9%	-0.9
pkg/reconciler/taskrun/resources/taskref.go	85.7%	84.9%	-0.8
pkg/resolution/resolver/bundle/params.go	75.9%	78.1%	2.3
pkg/resolution/resolver/cluster/resolver.go	84.7%	86.1%	1.4
pkg/resolution/resolver/framework/fakeresolver.go	90.0%	91.7%	1.7
pkg/resolution/resolver/git/resolver.go	84.6%	84.9%	0.3
pkg/resolution/resolver/hub/resolver.go	70.6%	73.7%	3.1

abayer · 2022-09-19T16:59:33Z

/assign @jerop @lbernick @vdemeester @pritidesai

abayer · 2022-09-19T17:03:40Z

/retest

abayer · 2022-09-20T15:03:31Z

/hold

After discussion with @vdemeester about whether we should stop having the separate resolvers.yaml and instead include config/resolvers in release.yaml (which will let us assume that the resolvers are installed and enabled by default, which is important for things like the deprecation/eventual removal of taskRef.bundle and ClusterTask in favor of their resolver equivalents), I've made some changes (currently in a separate commit):

release.yaml does now include config/resolvers.
A new minimal-release.yaml will be created, with config/resolvers excluded from it, matching the contents of release.yaml as of v0.40.x.
resolvers.yaml will still be created, but is only needed/used if you want to add the built-in resolvers after installing Pipelines with minimal-release.yaml.
Docs are updated accordingly.

tekton-robot · 2022-09-20T15:05:22Z

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/v1/pipeline_types.go	95.7%	95.5%	-0.2
pkg/apis/pipeline/v1beta1/pipeline_types.go	96.8%	96.7%	-0.1
pkg/reconciler/pipelinerun/resources/pipelineref.go	89.7%	88.9%	-0.9
pkg/reconciler/taskrun/resources/taskref.go	85.7%	84.9%	-0.8
pkg/resolution/resolver/bundle/params.go	75.9%	78.1%	2.3
pkg/resolution/resolver/cluster/resolver.go	84.7%	86.1%	1.4
pkg/resolution/resolver/framework/fakeresolver.go	90.0%	91.7%	1.7
pkg/resolution/resolver/git/resolver.go	84.6%	84.9%	0.3
pkg/resolution/resolver/hub/resolver.go	70.6%	73.7%	3.1

afrittoli · 2022-10-05T09:15:19Z

Notes about things we should implement for remote resolution (not in this PR):

Better support in CLI: display task name, display linked request resolution, tkn requestresolution command or at least better default columns when using kubectl:

➜ k get resolutionrequests
NAME                                   SUCCEEDED   REASON
git-40e5840171b418bcbd0bfa73defec338   True
git-6ecf81c8e0b418bcbd0c05c1bc3cd0c5   True

Better support in Dashboard: display task name, ability to view request resolutions
Updated Tekton aggregated cluster roles

afrittoli

I'm going through the review - so far so good, and sorry about the late review 😓

I'm slightly concerned about moving to beta a feature that we have not tried in dogfooding.
The standard resolution mechanism is still there, so this is probably ok.
Before we use this to fully replace existing features like ClusterTasks we'll need to get a bit more experience with the feature though.

Also I think we need slightly better YAML test coverage.

We have no single yaml test in CI which with a PipelineTask that uses remote resolution.
We do have tests of pipelines that use remote resolution but they are excluded form CI. At least for the git resolver it should be easy to host a test pipeline in the pipeline repo?

docs/bundle-resolver.md

tekton/publish.yaml

afrittoli · 2022-10-05T10:44:14Z

tekton/publish.yaml

      koparse \
-        --path $OUTPUT_RELEASE_DIR/release.yaml \
+        --path $OUTPUT_RELEASE_DIR/minimal-release.yaml \


Why do we need two parameters $IMAGES and $RESOLVER_IMAGES and calling koparse twice?
Couldn't this be condensed into one now that the default release file includes all images?

@vdemeester is the one to ask. =)

So, today, we are shipping release.yaml and resolvers.yaml. This would make it so that release.yaml has the resolvers by default, but I would like to keep a release.yaml that doesn't have those as well (and thus also keep resolvers.yaml).

Maybe instead of using ko resolve multiple time, we could use yq to generate new files with the thing we want to filter out. (And this would also be true for the -t as we could remove the tag part from it using yq).

My question was not about koparse, not ko resolve actually.
The ko resolve calls should be fine, since the built images will be in the cache so it won't actually build the same image more than once.

The koparse is not a big deal, but I would prefer a simpler script / logic where possible.
The split was done because of v0.40, now we could revert that and go back to one koparse invocation and fewer params in the pipeline/task.

oh boy, yq? I have no idea what to do there. =) If we can't do it via multiple koparse etc calls, I'd really prefer we just have release.yaml with everything in it.

Ok, for now, I'd say let's keep minimal-release.yaml etc, and not let that question block this PR, but make a final decision before v0.41 releases and if we decide to just have release.yaml, I'll do a quick followup PR to remove minimal-release.yaml and resolvers.yaml. @afrittoli @vdemeester does that sound good?

I've opened #5607 for that discussion.

docs/resolution-getting-started.md

go.sum

pkg/apis/pipeline/v1/pipeline_types.go

abayer · 2022-10-05T11:19:12Z

We have no single yaml test in CI which with a PipelineTask that uses remote resolution.
We do have tests of pipelines that use remote resolution but they are excluded form CI. At least for the git resolver it should be easy to host a test pipeline in the pipeline repo?

Whoops! I meant to add a test using test/git-resolvers/remote-task.yaml - I'll do that in a separate PR I'll open shortly.

And done - #5604

tekton-robot · 2022-10-05T11:50: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/pipeline/v1beta1/pipeline_types.go	96.8%	96.7%	-0.1
pkg/reconciler/pipelinerun/resources/pipelineref.go	89.7%	88.9%	-0.9
pkg/reconciler/taskrun/resources/taskref.go	85.7%	84.9%	-0.8
pkg/resolution/resolver/bundle/params.go	75.9%	78.1%	2.3
pkg/resolution/resolver/cluster/resolver.go	84.9%	86.2%	1.3
pkg/resolution/resolver/framework/fakeresolver.go	90.3%	91.9%	1.6
pkg/resolution/resolver/git/resolver.go	84.0%	84.2%	0.3
pkg/resolution/resolver/hub/resolver.go	69.2%	72.4%	3.2

afrittoli

Still going through the PR, still looking good, I left some comments.
I'm a bit confused about the alpha/beta flags in the reconciler and reconciler testing.
Also I would like to see in the PR/commit message/release notes all relevant changes introduced by the PR please 🙏

pkg/apis/resolution/v1beta1/resolution_request_types.go

pkg/reconciler/pipelinerun/pipelinerun_test.go

pkg/reconciler/pipelinerun/pipelinespec/pipelinespec.go

pkg/reconciler/pipelinerun/resources/pipelineref.go

pkg/reconciler/taskrun/resources/taskref.go

afrittoli · 2022-10-05T13:05:55Z

pkg/remote/resolution/resolver.go

@@ -34,7 +36,7 @@ type Resolver struct {
 	requester       remoteresource.Requester
 	owner           kmeta.OwnerRefable
 	resolverName    string
-	params          map[string]string


Could you add context in the commit/PR message about this change?
It doesn't seem to be strictly required for the v1beta1 version of remote resolution.
It would have been nice and easier to review to have that as a separate change before the v1beta1 🙏

pkg/resolution/resource/name.go

test/e2e-common.sh

Closes tektoncd#4710 There are two major aspects to this change: * Creating a `v1beta1` for `ResolutionRequest`, changing from `parameters` as a `map[string]string` to `params` (field name rename for `openapi-gen` API rule validation reasons) as a `[]pipelinev1beta1.Param`. This is being done to match `ResolverRef.Params`, which is a `[]pipelinev1beta1.Param`. We originally left `ResolutionRequest.Parameters` as `map[string]string` since it already was that in `v1alpha1` and changing it would require `ResolutionRequest` `v1beta1`, hence doing this now. Changing to `[]pipelinev1beta1.Param` is needed to support array and object params for resolvers, and we're going with a slice rather than a map because that's how you're supposed to do this in Kubernetes. =) * Promoting the remote resolution functionality from `alpha` to `beta`, meaning on by default for `v1beta1` `Pipeline`s etc, and requiring `enable-api-fields: beta` for `v1` `Pipeline`s etc. Additionally, we're undoing the questionable decision to split the resolvers out into `resolvers.yaml`, separate from `release.yaml`, and enabling the built-in resolvers. Deprecation of the legacy `taskRef.bundle` and `pipelineRef.bundle` syntax will be handled in a separate PR (tektoncd#5514). Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

lbernick · 2022-10-05T13:42:49Z

We have no single yaml test in CI which with a PipelineTask that uses remote resolution.
We do have tests of pipelines that use remote resolution but they are excluded form CI. At least for the git resolver it should be easy to host a test pipeline in the pipeline repo?

Whoops! I meant to add a test using test/git-resolvers/remote-task.yaml - I'll do that in a separate PR I'll open shortly.

There are a few examples tests that use a git clone copied directly from the catalog, e.g. https://github.com/tektoncd/pipeline/blob/main/examples/v1beta1/pipelineruns/pipelinerun-with-final-tasks.yaml. (It has the comment "This can be deleted after we add support to refer to the remote Task in a registry (Issue #1839) or add support for referencing task in git directly (issue #2298)". Maybe those can be replaced with git resolution?

abayer · 2022-10-05T13:46:56Z

@lbernick Yup, that's a followup. Right now I just have to figure out why #5604 is failing in CI but working for me locally. =)

abayer · 2022-10-05T13:53:37Z

@afrittoli Ok, so we do test a remote-resolved PipelineTask in https://github.com/tektoncd/pipeline/blob/main/test/resolvers_test.go#L202-L260 - I'm not sure why #5604 (adding an example with PipelineTask remote resolution) is failing, since it's passing for me locally. But I'll keep digging into it. =)

tekton-robot · 2022-10-05T14:22:10Z

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/pipeline_types.go	96.8%	96.7%	-0.1
pkg/reconciler/pipelinerun/resources/pipelineref.go	89.7%	88.9%	-0.9
pkg/reconciler/taskrun/resources/taskref.go	85.7%	84.9%	-0.8
pkg/resolution/resolver/bundle/params.go	75.9%	78.1%	2.3
pkg/resolution/resolver/cluster/resolver.go	84.9%	86.2%	1.3
pkg/resolution/resolver/framework/fakeresolver.go	90.3%	91.9%	1.6
pkg/resolution/resolver/git/resolver.go	84.0%	84.2%	0.3
pkg/resolution/resolver/hub/resolver.go	69.2%	72.4%	3.2

abayer · 2022-10-05T14:25:38Z

/retest

afrittoli · 2022-10-05T14:27:54Z

Thanks @abayer for working on this and for the updates.
Should I be worried about #5604 failing CI?

/hold cancel

abayer · 2022-10-05T14:36:51Z

@afrittoli Nah, pretty sure that's just due to GitHub webhooks having problems.

/retest

abayer · 2022-10-05T14:40:52Z

Rather, GH webhook issues are why the retest isn't firing - the unit test failure is just one of the family of event-related unit test flakes.

afrittoli · 2022-10-05T15:02:55Z

/lgtm

abayer added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 19, 2022

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

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 19, 2022

tekton-robot requested review from dlorenc and imjasonh September 19, 2022 14:50

abayer changed the title ~~Move remote resolution out of alpha~~ WIP: Move remote resolution out of alpha Sep 19, 2022

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 19, 2022

abayer force-pushed the resolution-v1beta1 branch from 03fa4ec to 6b88794 Compare September 19, 2022 15:10

abayer force-pushed the resolution-v1beta1 branch from 6b88794 to 5df2e62 Compare September 19, 2022 15:45

abayer force-pushed the resolution-v1beta1 branch from 5df2e62 to de4b925 Compare September 19, 2022 16:29

abayer changed the title ~~WIP: Move remote resolution out of alpha~~ Move remote resolution out of alpha Sep 19, 2022

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 Sep 19, 2022

tekton-robot assigned jerop, lbernick, pritidesai and vdemeester Sep 19, 2022

abayer added this to the Pipelines v0.41 milestone Sep 19, 2022

abayer force-pushed the resolution-v1beta1 branch from de4b925 to b0f4d2d Compare September 20, 2022 15:00

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2022

afrittoli mentioned this pull request Oct 5, 2022

Add extra display columns for resource resolution #5602

Merged

7 tasks

afrittoli reviewed Oct 5, 2022

View reviewed changes

abayer force-pushed the resolution-v1beta1 branch from 07039d6 to 707edb7 Compare October 5, 2022 11:45

afrittoli reviewed Oct 5, 2022

View reviewed changes

pkg/resolution/resource/name.go Show resolved Hide resolved

afrittoli reviewed Oct 5, 2022

View reviewed changes

test/e2e-common.sh Show resolved Hide resolved

afrittoli reviewed Oct 5, 2022

View reviewed changes

test/e2e-common.sh Show resolved Hide resolved

abayer force-pushed the resolution-v1beta1 branch from 707edb7 to 2bfe381 Compare October 5, 2022 13:36

abayer mentioned this pull request Oct 5, 2022

Should we support installing Pipeline without the resolvers? #5607

Open

tekton-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 5, 2022

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 5, 2022

afrittoli added the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2022

tekton-robot assigned afrittoli Oct 5, 2022

tekton-robot merged commit 7aa4a34 into tektoncd:main Oct 5, 2022

This was referenced Oct 11, 2022

Fix paramValue Type for bundle conversion to resolver #5625

Merged

Unhandled resolutionrequests in conversion Webhook #5631

Closed

Move remote resolution out of alpha #5515

Move remote resolution out of alpha #5515

Conversation

abayer commented Sep 19, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

abayer commented Sep 19, 2022

tekton-robot commented Sep 19, 2022

tekton-robot commented Sep 19, 2022

abayer commented Sep 19, 2022

tekton-robot commented Sep 19, 2022

tekton-robot commented Sep 19, 2022

tekton-robot commented Sep 19, 2022

abayer commented Sep 19, 2022

abayer commented Sep 19, 2022

abayer commented Sep 20, 2022

tekton-robot commented Sep 20, 2022

afrittoli commented Oct 5, 2022 • edited Loading

afrittoli 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abayer commented Oct 5, 2022 • edited Loading

tekton-robot commented Oct 5, 2022

afrittoli left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbernick commented Oct 5, 2022

abayer commented Oct 5, 2022

abayer commented Oct 5, 2022

tekton-robot commented Oct 5, 2022

abayer commented Oct 5, 2022

afrittoli commented Oct 5, 2022

abayer commented Oct 5, 2022

abayer commented Oct 5, 2022

afrittoli commented Oct 5, 2022

abayer commented Sep 19, 2022 •

edited

Loading

afrittoli commented Oct 5, 2022 •

edited

Loading

abayer commented Oct 5, 2022 •

edited

Loading

afrittoli left a comment •

edited

Loading