Disallow the Cluster and Git resource types as Output resources. #1109
Conversation
The cluster type currently has no defined behavior as output resources, and the git type behaves incorrectly. The new Volume type should be used instead for the use-cases supported by the old Git resource. This is part of the large cleanup in tektoncd#1076, and should not be submitted until after tektoncd#1087.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dlorenc 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 |
|
The following is the coverage report on pkg/.
|
|
@dlorenc: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
🎉 I am excited to get this ball rolling!! 🎉 imo this counts as a backwards incompatible change, in which case I think we'd want to do something like:
I think (as you're seeing in the test failures) that it might be impossible to do input output linking in our tests with out this? (i think to use GCS we would need to surmount #821 🤔🤔🤔 Also need to update:
(I'm definitely for doing this in the long run! I think this is gonna be a pretty disruptive change to anyone using linking tho since using a |
|
I agree with @bobcatfish - Jenkins X's pipelines all utilize |
|
/hold |
There was a problem hiding this comment.
- Can has doc update? https://github.com/tektoncd/pipeline/blob/master/docs/resources.md
- Can we add a follow up issue to add support for Git resources as outputs? (i.e. do a commit back to the repo in that case)
- Some examples and end to end tests are definitely going to need an update - maybe you're waiting for the volume type before then tho? the DAG test in particular is totally relying on using git inputs - we might be able to replace it with a storage buckets but from We aren't running bucket storage-related e2e tests in CI #821 it seems like we've been having some trouble with that
| // ResourceFromType returns a PipelineResourceInterface from a PipelineResource's type. | ||
| func ResourceFromType(r *PipelineResource) (PipelineResourceInterface, error) { | ||
| // InputResourceFromType returns a PipelineResourceInterface from a PipelineResource's type. | ||
| func InputResourceFromType(r *PipelineResource) (PipelineResourceInterface, error) { |
There was a problem hiding this comment.
would it be crazy in Go to define a top level like map of types to New functions? e.g. something like:
PipelineResourceTypeGit: NewGitResource,
PipelineResourceTypeImage: NewImageResource,If case statements are clearer, just ignore me
| case PipelineResourceTypeCluster: | ||
| return NewClusterResource(r) | ||
| default: | ||
| return InputOutputResourceFromType(r) |
There was a problem hiding this comment.
i found it confusing that the default for this function calls InputOutputResourceFromType - id find it a bit easier to see explicitly which types are allowed for input and output, and always default to an error
In tektoncd#1109 we will be removing support for git as an output. In the current implementation, git as an output is just a volume that holds the data from the git repo, and copies it between Tasks (when git as an output is linked to git as an input). As discussed in tektoncd#1076, the model we want for PipelineResources is for them to take the outside world and represent it on disk when used as an input, and when used as an output, to update the outside world. In order to do this, what we actually want for a git output is for it to create a commit the repo it is referencing. However up until this point folks have been using git resources in the way that we want Volume Resources to behave tektoncd#1062, so we want to transition folks to Volume Resources and away from using git outputs. Fixes tektoncd#1283
|
@sbwsg and @dlorenc have a new design to propose for #1272 and #1076 which I think will make this change unnecessary since they are proposing overhauling the way we express these resources completely, so I'm going to close this PR for now, but we can re-open if we decide we want to keep making these incremental improvements to PipelineResources as they are today (see also #1062 (comment)) |
|
p.s. I may open a PR to disallow cluster outputs anyway since that doesnt really make sense |
Changes
The cluster type currently has no defined behavior as output resources, and
the git type behaves incorrectly. The new Volume type should be used instead
for the use-cases supported by the old Git resource.
This is part of the large cleanup in #1076, and should not be submitted until
after #1087.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
Release Notes