tektoncd
Add types and client for Resolution
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
- [x] Has Tests included if any functionality added or changed
- [x] Follows the commit message standard
- [x] Meets the Tekton contributor standards (including functionality, content, code)
- [x] 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 - [x] Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
- [x] 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: 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.
/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.
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 |
@pritidesai @vdemeester I've changed the resource: syntax to be
resource:
foo: bar
instead of
resource:
- name: foo
value: bar
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 |
/hold
Putting on hold again until we nail down whether we keep the resource field name or not.
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 |
I've added a commit changing resource: to params: - if it seems good to everyone, I'll squash it, and open a corresponding PR to update the TEP.
OK I'm sorry I don't really have a better suggestion here, but params might be confusing since the syntax was changed to be non-params-like
@lbernick Yeah, I don't like params either, but can't think of anything better. values doesn't feel right...maybe args? Though it's not the same as a container/step's args...
You know what? I'm just going to go with paramsMap for now - yeah, it's still got params in it, but I feel like being a different string, with map explicitly in the string, is good enough.
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 |
No worries! =) I'm not in love with any option at this point, though I do prefer paramsMap to either params or resource, hence me updating the PR.
Another option could be resolverParams - which stutters a bit being under the resolver field. Again, I'm open to literally any suggestion.
So after discussion with @lbernick and her discovery of https://github.com/tektoncd/pipeline/pull/4502#discussion_r803199842, I've come to the conclusion that we should stick with the subobject syntax, with the field renamed to params. As discussed in that comment, this is more consistent with Kubernetes in general, and it also will make it much easier to add support for array/object parameters in the future if we so desire. That will entail a change to ResolutionRequestSpec.Parameters to not be a map[string]string as well, and an update to the TEP.
cc @pritidesai @vdemeester
So after discussion with @lbernick and her discovery of #4502 (comment), I've come to the conclusion that we should stick with the subobject syntax, with the field renamed to
params. As discussed in that comment, this is more consistent with Kubernetes in general, and it also will make it much easier to add support for array/object parameters in the future if we so desire. That will entail a change toResolutionRequestSpec.Parametersto not be amap[string]stringas well, and an update to the TEP.cc @pritidesai @vdemeester
So, I agree that, if we stick with params field for this, we should be consistent with the shape of params in the other parts of the API. I disagree that we should be necessarily consistent with Kubernetes on this. We should aim for the best UX for our users not because Kubernetes does it — and we are trying to not be too much "tied up" with Kubernetes as well. If we aim for end-users to author Pipeline, Task, … we should aim for making the API tailored for them, and not necessarily follow what idiomatic Kubernetes API does/looks like.
The params syntax is quite verbose. If we use the bundle for comparison :
# from
taskRef:
bundle: quay.io/vdemeest/foo:bar
name: baz
# to
taskRef:
resolver: bundle
params:
- name: image
value: quay.io/vdemeest/foo:bar
- name: name
value: baz
- name: type
value: Task
From a user perspective, an "author", I have the feeling that the following feels better.
taskRef:
resolver: bundle
params:
image: quay.io/vdemeest/foo:bar
name: baz
type: Task
That might be one of the reason the TEP uses resource instead, to not have to be consistent with params.
On "additionnal" point here, the resulting ResolutionRequest type will have a different params syntax as the one specified in resolver. Do we want to change this as well ?
apiVersion: resolution.tekton.dev/v1alpha1
kind: ResolutionRequest
# […]
spec:
params:
serviceAccount: default
bundle: gcr.io/tekton-releases/catalog/upstream/golang-build:0.1
name: golang-build
kind: task
EOF
In a gist, I am not a huge fan of using params and the task.params syntax, and I would rather stick to the proposed TEP syntax 😅 ; mainly because, from a user/author perspective, it's easier to write 😇. Sorry for the back and forth 😓
On "additionnal" point here, the resulting
ResolutionRequesttype will have a differentparamssyntax as the one specified inresolver. Do we want to change this as well ?
Eventually, yes, but I think that to do that right, it will involve moving ArrayOrString out of pkg/apis/pipeline so we can use it in pkg/apis/resolution as well. Since that will be a decent amount of work, and ResolutionRequests aren't user-authored anyway, I thought that work could wait til later.
In a gist, I am not a huge fan of using
paramsand thetask.paramssyntax, and I would rather stick to the proposed TEP syntax 😅 ; mainly because, from a user/author perspective, it's easier to write 😇. Sorry for the back and forth 😓
I don't disagree with this in principle, but I do in practice - we've already got the params syntax all over the place, and we're definitely going to want to end up supporting array parameters for resolvers (and while I see less value in object parameters, we'll end up getting that for free if/when we end up moving to using ArrayOrString to get array parameter support). So the choice is not "use an admittedly subpar/overly verbose syntax for resolver.[params|resource] or just use maps", it's "use the same admittedly subpar/overly verbose syntax for resolver.[params|resource] that we do elsewhere or use a different syntax unique to this one field to accomplish the same end result".
it's "use the same admittedly subpar/overly verbose syntax for
resolver.[params|resource]that we do elsewhere or use a different syntax unique to this one field to accomplish the same end result".
I agree with that 💯 % 😝. But if we didn't name it params, but something else, maybe spec 😈. Then we would have something completely "new" and "unique", and more closely to the spec you tend to see in Kubernetes. For example volumeClaimTemplate, etc.. I agree it's a very thin line between parameters and the "spec" of the resolver though 😝.
I agree with that 💯 % 😝. But if we didn't name it
params, but something else, maybespec😈. Then we would have something completely "new" and "unique", and more closely to thespecyou tend to see in Kubernetes. For examplevolumeClaimTemplate, etc.. I agree it's a very thin line between parameters and the "spec" of the resolver though 😝.
Right - so I think that's a legitimate argument for ResolutionRequest's Params field, since that's in a different CRD. But within the context of a pipelineRef or taskRef, I think it would be a mistake to have a different syntax for .resolver.[params|resource] than we do elsewhere in the same Pipeline/PipelineRun/TaskRun CRD.
EDIT: Also, it doesn't really feel like a spec to me, because the field names aren't defined in a CRD somewhere, they're arbitrarily named parameters. There's a reasonable case to be made that the various resolver implementations should actually have their own spec types, etc, but that's a messy challenge to deal with since we don't know all the potential resolvers that could be used/written/etc. So...no, I'd stay with params.
In general, I'd argue that we should only have one syntax in Pipeline for arbitrarily-named key/value pairs, and since we already have one (params and results etc using - name: ...), we should keep using that one syntax in all cases.