tektoncd
Handle `PipelineResources` when swapping `v1` to the storage version
Decisions have been made both under this discussion and during the WG as we are moving forward with removing the pipelineResources :
- [x] remove cloudevent resources
- [x] remove cluster resources
- [x] remove kubeconfigwriter image
- [x] remove pullrequest resources
- [ ] image
- [ ] storage
- [ ] git
PipelineResources was deprecated but not removed from v1beta1 according to https://github.com/tektoncd/pipeline/issues/5785 and the WG decisions at Apr 11, 2022 and confirmed at Nov 28, 2022.
However when swapping v1 to be the storage version, the PipelineResources related logics could not be handled by changing v1beta1 to v1. eg. for the taskRun reconciler validate_resources.go would not be valid if we swap to v1.TaskSpec as PipelineResources are removed during the migration to v1.
One approach would need to be decided about handling the deprecated but not removed fields as PipelineResources for reconciling.
/kind misc subTask of https://github.com/tektoncd/pipeline/issues/5541
One thing to consider, because those fields are in v1beta1, we can remove them from v1beta1 (given the proper warning, deprecation notice, …). Doing this "just before" we switch to v1 would help us a lot 👼🏼 to not make too much complexity for a deprecated feature.
Yeah pipeline resources have been deprecated since v0.30 so I think it can be removed
Yes please, let's remove from the code base which will give us an opportunity to comply the use cases mentioned in the TEP. At the same time, an opportunity to reconsider and design features which could potentially replace resources - https://github.com/tektoncd/community/blob/main/teps/0074-deprecate-pipelineresources.md#features-that-will-replace-pipelineresources-functionality
Also updated TEP0074 at https://github.com/tektoncd/community/pull/922 Thanks for the reminder cc @lbernick
Since CloudEvents as for CloudEventDeliveryState shall be also removed according to V1 TaskRun Golang structs change. Therefore, shall we make an exception at deprecations.md as the CloudEvents should have been deprecated along with the pipelineResources?
I don't like the idea of backdating our deprecation docs to say that we deprecated something 9 months ago, but I agree that taskrun.status.cloudevents is pretty meaningless without pipelineresources, and it doesn't make sense to wait 9 months after "deprecating" taskrun.status.cloudevents before removing PipelineResources. Maybe we can update the PipelineResources line of the deprecation docs to note that it includes cloudevents, and remove release notes from https://github.com/tektoncd/pipeline/pull/5937 to avoid confusion?
I don't like the idea of backdating our deprecation docs to say that we deprecated something 9 months ago, but I agree that taskrun.status.cloudevents is pretty meaningless without pipelineresources, and it doesn't make sense to wait 9 months after "deprecating" taskrun.status.cloudevents before removing PipelineResources. Maybe we can update the PipelineResources line of the deprecation docs to note that it includes cloudevents, and remove release notes from https://github.com/tektoncd/pipeline/pull/5937 to avoid confusion?
Agreed on the drawback of backdating deprecation doc for the mentioned reason. Will instead work on updating and adding new instructions for deprecation as suggested.
Small-scale design for planing out and evaluating different approaches:
- https://github.com/tektoncd/pipeline/pull/6000
- This makes the removal a large chunk and violates the code standard of
small PRs- This would require long time to review and hard to roll back
- Start removing
inputsandoutputsfrom reconciler structs:
- This is slightly smaller than https://github.com/tektoncd/pipeline/pull/6000, could be an approach after 3 is done.
- Remove resources and respective tests first:
- https://github.com/tektoncd/pipeline/pull/5995
- https://github.com/tektoncd/pipeline/pull/5996
- This would remove the test cases in reconciler first and breaks down the PR which contains them in https://github.com/tektoncd/pipeline/pull/6000
I personally tend to move forward with the last approach as it is more efficient and easier to rollback.
There is also gsutil image and pullrequest-… image that could be removed (and the PullRequest PipelineResource as well).
As in https://github.com/tektoncd/pipeline/pull/5995#pullrequestreview-1260790448, we also want to decide and implement the deprecation as long as the removal of pipelineResources related APIs:
- resources inputs/outputs
- resource results
- cloudevenets status
These will be in the follow up PRs other than the removals of the pipelineResources themselves.
** Also putting this into the pipeline WG for discussions.
@tektoncd/core-maintainers There's some discussion going on in #6011 about whether to deprecate the "pull request" catalog task that depends on the "pullrequest-init" image. We also already removed the kubeconfigwriter image in https://github.com/tektoncd/pipeline/pull/5996, which is used in the "kubeconfig-creator" task. TEP-0074 states that we'll move the code for these images into a separate repo so the catalog tasks can continue using them, since these catalog tasks are our recommended way to migrate off of pipelineresources.
It seems like the discussion in #6011 is leaning towards deprecating these tasks, which might be a good idea since they seem to be unmaintained-- but we recommend these tasks for people moving off of these pipelineresources, and I wouldn't want to leave people without a migration strategy.
Maybe the best solution for right now is to continue to leave the code for these images in place, but remove the code for PipelineResources? Not sure if it's worth it to bring the kubeconfigwriter image back. Happy to update the TEP either way.
Thanks @lbernick for moving this forward
Maybe the best solution for right now is to continue to leave the code for these images in place, but remove the code for PipelineResources?
Can we confirm if this suggests reverting https://github.com/tektoncd/pipeline/pull/5996 and closing https://github.com/tektoncd/pipeline/pull/6002 since the images could not be retained without those resources themselves?
I am not sure if it is possible to just extract specific images that some catalog tasks relied on, for example as https://github.com/tektoncd/pipeline/pull/6011#issuecomment-1398843313 suggested:
The [git-init](https://github.com/tektoncd/pipeline/tree/main/cmd/git-init) images will be used by git-batch-merge and git-clone tasks, so I will extract the [git-init](https://github.com/tektoncd/pipeline/tree/main/cmd/git-init) to the new [git-clone](https://github.com/tektoncd-catalog/git-clone) catalog repo as part of the migration.
Can I confirm if this suggests reverting #5996 and closing #6002 since the images could not be retained without those resources themselves?
I am not sure if it is possible to just extract specific images that some catalog tasks relied on as suggested at #6011 (comment)
My understanding is that the PipelineResources use those images, not the other way around, so we can still build and release these images without continuing to support PipelineResources. Let's wait to hear what others think before reverting the removal of the kubeconfigwriter image. You could update #6002 to not remove the "pullrequest-init" image. From the comment linked it sounds like Quan plans to move the git-init image out of pipelines, but I don't think that affects our plans to remove support for PipelineResources.
Opened https://github.com/tektoncd/community/pull/940 proposing we deprecate these images (other than git-init).
Can I confirm if this suggests reverting #5996 and closing #6002 since the images could not be retained without those resources themselves? I am not sure if it is possible to just extract specific images that some catalog tasks relied on as suggested at #6011 (comment)
My understanding is that the PipelineResources use those images, not the other way around, so we can still build and release these images without continuing to support PipelineResources. Let's wait to hear what others think before reverting the removal of the kubeconfigwriter image. You could update #6002 to not remove the "pullrequest-init" image. From the comment linked it sounds like Quan plans to move the git-init image out of pipelines, but I don't think that affects our plans to remove support for PipelineResources.
One question for removing the image resource at https://github.com/tektoncd/pipeline/pull/6002 is that seems the dependencies is at https://github.com/tektoncd/pipeline/pull/6002 where the resources itself is being removed. Then in this case, there is no way other than extracting it to keep the imagedigestexporter image without image resources?
Can we please tag this to v0.45 milestones as it shall target the next release? Thanks @tektoncd/core-maintainers
The last decision to be made for removing Image and Storage + Git resources:
- Remove Storage + Git first since it has already been a large chunk of 11,000+ lines of deletion; then remove
Imageresources along with the generic resources functions - Remove Image Resources first and then remove Storage + Git with the generic funcions.
The 2nd option might be preferred as it could be more efficient.
The Git and Storage PipelineResources are removed within the same pull request because the git resources also uses artifact storage. Therefore, so as not to make it a larger chunk, shall we consider removing https://github.com/tektoncd/pipeline/pull/6002 as the last pipelineResource type along with the removal of the generic pipelineResources functions?
This comment aims to tracks the progress of this issue and mitigate the confusions of the decisions being made on the sequence of these removals:
- At first we targeted to remove
git resourcesat last since most generic functions have used Git as a valid type, remove the generic resources functions with remove git resources - But then according to comment at https://github.com/tektoncd/pipeline/pull/6014#discussion_r1104486051, I thought it might worth removing Git and Storage and artifacts together
- Then it comes to this stage that we tried to remove image pipelineResources at the end, bundling it with the removal of the generic pipelineResources functions and remove Git + storage + artifacts in https://github.com/tektoncd/pipeline/pull/6150
Trying to keep PRs small and review-able so looking back on Reasons why removing Git, Storage Resources andpkg/artifacts together:
Also in the PR comment of https://github.com/tektoncd/pipeline/pull/6150:
At the beginning I also attempted to start from the smallest PR possible but changed directions inevitably during the process of impelmentations.
- Why bundle Git and Storage resources removal? The Git and Storage PipelineResources both uses artifact storage, which made removing Storage PipelineResources along to be intervened by changes on Git Resources that are inappropriate to separate. See the decision made after attempting to remove Storage Resources along for more detail.
- Why bundle removal of Storage resources with the
artifactspackage? The artifacts package has also dependencies on storage resources for the artifacts storage setup, which made it hard to separate the removal of storage resources first. See the decision after the attempt to remove artifacts package first.
Please feel free to point out if there are any way to break this PR down or if there are any negligence above.
Some questions that might need consensus to be reached before merging the PRs:
- Is it acceptable to leave generic functions uncovered of testing with all pipelineResource Types removed (since all actual resource types are removed, many test cases would be gone)?
- The sequence of removing the rest to be decided as long as keeping the PR to be small as possible:
- image
- git
- storage
- artifacts pkg
- generic resources fucntions
Another question encountered when is when would we extract the git-init image. It seems the removal of generic pipeline-resources functions are blocked by the extraction of the git-init image as the pipelineResourceResults are needed here by the git-init image.
And also thanks to the pointers from @pritidesai that for this is also used by spire, currently looking into how this shall be moved forward.
It has been previously used in:
- termination package https://github.com/tektoncd/pipeline/blob/main/pkg/termination/write.go#L33
- pod status https://github.com/tektoncd/pipeline/blob/main/pkg/pod/status.go#L258
- spire https://github.com/tektoncd/pipeline/blob/main/pkg/spire/spire_mock.go#L181
Another question encountered when is when would we extract the git-init image. It seems the removal of generic pipeline-resources functions are blocked by the extraction of the git-init image as the pipelineResourceResults are needed here by the git-init image.
And also thanks to the pointers from @pritidesai that for this is also used by spire, currently looking into how this shall be moved forward.
I'm not sure if I fully understand, but I don't think the removal of git-init image should be blocked by the "extraction" right? i.e. we can remove the source code of git-init from the pipeline repo now, and migrate the source code to a separate repo from the old revision of the pipeline repo when we have time.
The PipelineResourceResults type is a bit "poorly" named nowadays (but well, it was introduced as part of PipelineResource intially) but it can definitely be kept and used independently of PipelineResource. I tend to agree with @QuanZhang-William, we don't necessarily have to extract it 🤔
I also agree with @QuanZhang-William on the rest 🙃
Thanks @vdemeester and the offline discussion with @QuanZhang-William . Opened https://github.com/tektoncd/pipeline/issues/6197 as a subtask of the current epic.
@JeromeJu please help evaluate the status of this epic. I am moving it to 0.47 for now but looks like all the tasks are marked as done. Are all the PRs merged for each task in the PR description?