Fix PipelineResource name handling. #2697
Conversation
tekton-robot
added
the
size/L
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
1 similar comment
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
The following is the coverage report on the affected files.
|
|
/kind bug |
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbwsg 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:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
hi all! can we get the release notes field filled out? just cruising by this one and i want to understand the user facing impact (if any - it seems like at least a bug fix tho?) |
|
The following is the coverage report on the affected files.
|
It's a bug fix that sort of requires an API change. Release Notes should be filled in now, we might want to discuss this in next week's meeting though before merging. |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
@dlorenc needs a rebase 😅 |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
A couple quick requests (probably):
- Could you update some docs with the new field? probably in https:/tektoncd/pipeline/blob/master/docs/taskruns.md#monitoring-execution-status (apologies that there isnt much there to start with)
- If we want to deprecate the old field, could you add it to https:/tektoncd/pipeline/blob/master/docs/deprecations.md - i also want to double check, do we definitely want to remove it? it might make sense to have both? (or maybe it ultimately would make more sense to embed the resourcespecs in the status in the same way as we do the task spec? in that case i think deprecating the field makes sense - though you still might want to be able to map the task's resource name to the actual resource name used? ... i guess that's already in the spec?)
My one concern is that based on how the cluster end to end test changed, is this a backwards incompatible change for the behavior of the cluster pipelineresource, or was it just a quirk of how the test was setup? If the latter then I think we're all good, but if it is a backwards incompatible change we might want to do it a bit more slowly.
Yes, we definitely want to delete this. It doesn't make semantic sense for the ResourceResult to contain a ResourceRef for a few reasons:
Instead it should probably contain a fully dereferenced copy of the resource (like you've suggested), but that should wait until we figure out the plan for resources. |
And yes, this is slightly backwards incompatible. I just changed the test to be more clear. The old behavior for ClusterResources was just plain wrong - they were writing to the wrong location on disk. Referring to the location via $(resources.inputs.) still works and provides stable behavior. I've explained this in the notes. I don't think there's a lot more we can do here... |
|
OK, this has now been changed (and the release notes are updated) to keep both paths in place for now. We can remove the old path in the next release. |
|
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Thanks @dlorenc ! A couple minor requests around the deprecation doc, also want to make sure we have a test or something that ensures the deprecated version continues to work
|
The following is the coverage report on the affected files.
|
In discussion around tektoncd#2697, we realized the existing API compatiblity policy is a little unclear, leading to confusion. We discussed and clarified the intentions of this policy in tektoncd#2790. This change adds an example to the policy, and tweaks the deprecations table to make a bit more sense. Fixes tektoncd#2790
In discussion around #2697, we realized the existing API compatiblity policy is a little unclear, leading to confusion. We discussed and clarified the intentions of this policy in #2790. This change adds an example to the policy, and tweaks the deprecations table to make a bit more sense. Fixes #2790
OK, this should be ready now. |
This commit changes the names used throughout PipelineResource handling. PipelineResources have two names when used in a TaskRun: - The name of the PipelineResource entity (external name) - The name given to the resource inside the Task definition (internal name) Task code is currently using the external names everywhere, but should be using the internal names. This adds a new field for the internal name to PipelineResourceResult, and deprecates the existing PipelineResourceResult.ResourceRef field. Ref: tektoncd#2694
|
The following is the coverage report on the affected files.
|
|
/lgtm |
|
In response to this:
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. |
…release. This is a cleanup commit that should go in after 0.14.0 is cut. It was introduced in tektoncd#2697, to fix tektoncd#2694.
Changes
This commit changes the names used throughout PipelineResource handling.
PipelineResources have two names when used in a TaskRun:
Task code is currently using the external names everywhere, but should be using
the internal names.
Ref: #2694
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes