Add types and client for Resolution #5200

abayer · 2022-07-22T16:33:04Z

Changes

Part of #4710

In order to get remote resolution functionality to beta, we're moving it from a separate repository/release to be part of Pipeline. This is the first PR in a sequence moving the code from the Resolution repository into Pipeline.

/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: Remote resolver `resource` field has changed to `params`.

abayer · 2022-07-22T16:33:20Z

/assign @vdemeester @dibyom
cc @chitrangpatel

abayer · 2022-07-22T16:34:23Z

/test check-pr-has-kind-label

tekton-robot · 2022-07-22T16:34:24Z

@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.

abayer · 2022-07-22T16:48:31Z

/hold

Putting this on hold until we've got v0.38.0 cut, 'cos I'd really like to have all of the relevant commits in one release together.

pritidesai · 2022-07-22T18:33:26Z

pkg/apis/resolution/v1alpha1/resolution_request_types.go

+	// resource being requested. For example: repo URL, commit SHA,
+	// path to file, the kind of authentication to leverage, etc.
+	// +optional
+	Parameters map[string]string `json:"params,omitempty"`


I was sharing the remote resource spec with a team and came across two different syntaxes. It seems we are implementing it using params:

taskRef: resolver: git resource: - name: url value: https://github.com/tekton/catalog.git - name: path value: /task/golang-fuzz/0.1/golang-fuzz.yaml - name: branch value: main

The proposal in the Remote Resolution TEP has listed it as:

taskRef: resolver: git resource: repository_url: https://github.com/tektoncd/catalog.git branch: main path: /task/golang-fuzz/0.1/golang-fuzz.yaml

Do we need to update the proposal to reflect params? or is it documented somewhere else?

Good question! I didn't do the initial implementation, but while I like the repository_url etc syntax better, the - name: url syntax is simpler to implement, since we don't need to use a runtime.RawExtension here for arbitrary YAML.

So I'd say the proposal should be updated, yeah.

@abayer to play devil's advocate, we should aim for the API to be the easiest to use (for end-user) than the easiest to implement 😝

Fair point. =) I spent some time this weekend thinking about this - in practice, it probably is easier to go with a simple map[string]string than I'd been thinking, given that what we've got now is already just a slice of string name/value pairs anyway. Then, if we hit a situation down the road where we have a need for more complex resolver parameters, we could change that to something like unstructured.Unstructured without breaking API compatibility.

...though it is worth mentioning that changing the syntax will break compatibility with the existing syntax, but that's alpha syntax and probably not at all widely used right now anyway.

👍 Lemme do that!

the reason why it was implemented as such was because of K8s convention: "Lists of named subobjects preferred over maps" - as discussed in https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#lists-of-named-subobjects-preferred-over-maps and kubernetes/kubernetes#2004

and we listed in https://github.com/tektoncd/community/blob/main/design-principles.md#api-conventions that Tekton API should comply with the K8s API conventions

haha just saw the comments below

and we listed in https://github.com/tektoncd/community/blob/main/design-principles.md#api-conventions that Tekton API should comply with the K8s API conventions

Yeah this is weird 😝 we want to not tied ourselves too much on kubernetes but still, we force ourselves to comply to kubernetes API convention 😅.

...and yeah, I reverted back to the old form for ResolutionRequest.Spec so as not to break compatibility. See #5200 (comment) =)

tekton-robot · 2022-07-25T15:31:38Z

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	85.7%	87.5%	1.8
pkg/reconciler/taskrun/resources/taskref.go	83.0%	84.1%	1.1

abayer · 2022-07-25T15:34:12Z

/retest

abayer · 2022-07-25T15:34:59Z

@pritidesai @vdemeester I've changed the resource: syntax to be

resource:
  foo: bar

instead of

resource:
  - name: foo
     value: bar

abayer · 2022-07-25T15:54:33Z

/retest

abayer · 2022-07-26T11:57:02Z

/hold cancel

vdemeester

/cc @tektoncd/core-maintainers

tekton-robot · 2022-07-26T13:47:05Z

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	85.7%	87.5%	1.8
pkg/reconciler/taskrun/resources/taskref.go	83.0%	84.1%	1.1

abayer · 2022-07-26T14:03:07Z

/retest

lbernick

thanks Andrew! I think we'll need an "action required" release note here

lbernick · 2022-07-26T14:30:08Z

pkg/apis/pipeline/v1beta1/resolver_types.go

-	// Value is the string value of the parameter that will be
-	// passed to the resolver.
-	Value string `json:"value"`
+	Resource map[string]string `json:"resource,omitempty"`


If we are making breaking changes here I'm wondering if "resource" is still the best thing to call this field? it's a very overloaded term in tekton.

Very good question! I am not a fan of resource here - it's not really clear what it means in the first place. I feel like just params would work, though since params is used in a bunch of places with different syntax than here, I'm not sure if that's ideal...

In the TEP, we used resource ? I'd rather go with params or values than resource indeed.

abayer · 2022-07-26T14:36:46Z

/hold

Putting on hold again until we nail down whether we keep the resource field name or not.

abayer · 2022-08-09T14:21:08Z

/retest

abayer · 2022-08-09T15:06:05Z

/retest

tekton-robot · 2022-08-16T13:17:45Z

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/resolver_conversion.go	37.5%	50.0%	12.5

abayer · 2022-08-18T13:00:05Z

So this still needs a squash, but I've avoided doing so until there's buy-in from @vdemeester. =)

I deliberately decided to not change the ResolutionRequest syntax for now, because there's already the v1alpha1 version in Resolution. Once this chain lands, I'll deprecate v1alpha1 and replace it with v1beta1 using []Param rather than map[string]string, but I didn't want to break compatibility for the existing CRD. We still need to have the v1alpha1 definition here so that we can add conversion to it, so that's why it's a bit messy. =)

tekton-robot · 2022-08-18T14:24:05Z

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/resolver_conversion.go	37.5%	50.0%	12.5

abayer · 2022-08-18T14:25:01Z

/retest

tekton-robot · 2022-08-18T14:39:23Z

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/resolver_conversion.go	37.5%	50.0%	12.5

tekton-robot · 2022-08-19T14:53: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/resolver_conversion.go	37.5%	50.0%	12.5

vdemeester · 2022-08-23T14:32:30Z

I deliberately decided to not change the ResolutionRequest syntax for now, because there's already the v1alpha1 version in Resolution. Once this chain lands, I'll deprecate v1alpha1 and replace it with v1beta1 using []Param rather than map[string]string, but I didn't want to break compatibility for the existing CRD. We still need to have the v1alpha1 definition here so that we can add conversion to it, so that's why it's a bit messy. =)

👍🏼

Alright, let's do the "Params as everywhere else approach". I don't think it's the ideal UX but it's the most constistent. Better have consistency 👍🏼

I approve 🎉

/approve

tekton-robot · 2022-08-23T14:32:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, lbernick, 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~~ [dibyom,lbernick,vdemeester]

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

vdemeester

Code LGTM

vdemeester · 2022-08-23T14:35:09Z

config/300-resolutionrequest.yaml

-    - all
-    - tekton


vdemeester · 2022-08-23T14:35:32Z

config/300-resolutionrequest.yaml

-          properties:
-            spec:
-              description: Spec holds the parameters for the request.
-              type: object
-              properties:
-                params:
-                  type: object


That's a shame this goes.. 😅

vdemeester · 2022-08-23T14:40:06Z

pkg/apis/resolution/v1alpha1/resolution_request_defaults.go

+import "context"
+
+// ManagedByLabelKey is the label key used to mark what is managing this resource
+const ManagedByLabelKey = "app.kubernetes.io/managed-by"


I feel we have this defined at least once or twice elsewhere 😝

We have - I tried to avoid having pkg/apis/resolution depend on anything from pkg/apis/pipeline, which may have been overkill.

vdemeester · 2022-08-23T14:41:08Z

pkg/apis/resolution/v1alpha1/resolution_request_types.go

+	// resource being requested. For example: repo URL, commit SHA,
+	// path to file, the kind of authentication to leverage, etc.
+	// +optional
+	Parameters map[string]string `json:"params,omitempty"`


and we listed in https://github.com/tektoncd/community/blob/main/design-principles.md#api-conventions that Tekton API should comply with the K8s API conventions

Yeah this is weird 😝 we want to not tied ourselves too much on kubernetes but still, we force ourselves to comply to kubernetes API convention 😅.

abayer · 2022-08-23T14:54:38Z

/hold cancel

tekton-robot · 2022-08-23T14:58:53Z

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/resolver_conversion.go	37.5%	50.0%	12.5

abayer · 2022-08-23T15:23:56Z

/retest

jerop

ArrayOrString was renamed to ParamValue in #5304 -- we need to rename it in this PR as well

Part of tektoncd#4710 In order to get remote resolution functionality to beta, we're moving it from a separate repository/release to be part of Pipeline. This is the first PR in a sequence moving the code from the Resolution repository into Pipeline. This also changes the `.resolver.resource` field's name to `.resolver.params` for consistency across our syntax. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

abayer · 2022-08-25T00:10:15Z

ArrayOrString was renamed to ParamValue in #5304 -- we need to rename it in this PR as well

Done!

tekton-robot · 2022-08-25T00:14:29Z

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/resolver_conversion.go	37.5%	50.0%	12.5

lbernick · 2022-08-25T13:32:26Z

/lgtm

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. labels Jul 22, 2022

tekton-robot requested review from dibyom and dlorenc July 22, 2022 16:33

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

tekton-robot assigned dibyom and vdemeester Jul 22, 2022

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

pritidesai reviewed Jul 22, 2022

View reviewed changes

abayer force-pushed the resolution-types branch from b11bf8b to 346d550 Compare July 25, 2022 15:26

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 Jul 25, 2022

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

vdemeester approved these changes Jul 26, 2022

View reviewed changes

tekton-robot requested a review from a team July 26, 2022 13:02

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

abayer force-pushed the resolution-types branch from 346d550 to 0109d1a Compare July 26, 2022 13:42

lbernick reviewed Jul 26, 2022

View reviewed changes

abayer force-pushed the resolution-types branch from ab51f0f to 75bdc56 Compare August 8, 2022 17:43

abayer force-pushed the resolution-types branch from 75bdc56 to 2045ac9 Compare August 16, 2022 13:13

abayer force-pushed the resolution-types branch from 2045ac9 to 4403414 Compare August 18, 2022 14:18

abayer force-pushed the resolution-types branch from 4403414 to de1605b Compare August 18, 2022 14:34

abayer force-pushed the resolution-types branch from de1605b to f7d0e57 Compare August 19, 2022 14:46

vdemeester reviewed Aug 23, 2022

View reviewed changes

abayer force-pushed the resolution-types branch from f7d0e57 to 30e9a8c Compare August 23, 2022 14:54

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

jerop reviewed Aug 24, 2022

View reviewed changes

abayer force-pushed the resolution-types branch from 30e9a8c to 744e4f3 Compare August 25, 2022 00:09

tekton-robot assigned lbernick Aug 25, 2022

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

tekton-robot merged commit 34a67ad into tektoncd:main Aug 25, 2022

Add types and client for Resolution #5200

Add types and client for Resolution #5200

Conversation

abayer commented Jul 22, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

abayer commented Jul 22, 2022

abayer commented Jul 22, 2022

tekton-robot commented Jul 22, 2022

abayer commented Jul 22, 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

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

tekton-robot commented Jul 25, 2022

abayer commented Jul 25, 2022

abayer commented Jul 25, 2022

abayer commented Jul 25, 2022

abayer commented Jul 26, 2022

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 26, 2022

abayer commented Jul 26, 2022

lbernick 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

abayer commented Jul 26, 2022

abayer commented Aug 9, 2022

abayer commented Aug 9, 2022

tekton-robot commented Aug 16, 2022

abayer commented Aug 18, 2022

tekton-robot commented Aug 18, 2022

abayer commented Aug 18, 2022

tekton-robot commented Aug 18, 2022

tekton-robot commented Aug 19, 2022

vdemeester commented Aug 23, 2022

tekton-robot commented Aug 23, 2022

vdemeester 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

abayer commented Aug 23, 2022

tekton-robot commented Aug 23, 2022

abayer commented Aug 23, 2022

jerop left a comment

Choose a reason for hiding this comment

abayer commented Aug 25, 2022

tekton-robot commented Aug 25, 2022

lbernick commented Aug 25, 2022

abayer commented Jul 22, 2022 •

edited

Loading