Handle PipelineResources when swapping v1 to the storage version

[JeromeJu]

Context

It has been way longer than the deprecation cycle since PipelineResources had been deprecated. Decisions have been made both under this discussion and during the WG as we are moving forward with removing all the pipelineResources from v1beta1 code base (see 2023 Jan06 update for more details).

This issue aims to track the progress and the implementation details of tackling this v1 migration blocker.

Work plan

Original work plan:

• Remove all resources within one release:
  • Pros: avoid confusion from users to see that there are part of the PipelineResources removed while others are still supported.
  • Cons: One huge PR for making the breaking changes.
• remove cloudevent resources
• remove cluster resources
  • remove kubeconfigwriter image
• remove pullrequest resources
  • remove pullrequest-init image
• image
  • migrate Kaniko Task Pipelinerun examples off ImageDigestExporter
• storage
• git

---

[2023 Jan06] update

PipelineResources was deprecated but not removed from v1beta1 according to #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.

Expected work flow

• Solicit feedback from users and WG
  • WG discussion @nov 28
  • Update deprecation doc and user-facing documentations.
• POC
• Implementation

/kind misc
subTask of #5541

[vdemeester]

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.

[dibyom]

Yeah pipeline resources have been deprecated since v0.30 so I think it can be removed

[pritidesai]

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

[JeromeJu]

Also updated TEP0074 at tektoncd/community#922
Thanks for the reminder cc @lbernick

[JeromeJu]

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?

[lbernick]

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 #5937 to avoid confusion?

[JeromeJu]

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

[JeromeJu]

Small-scale design for planing out and evaluating different approaches:

1. [WIP] Remove pipelineResources in a Single Attempt #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

2. Start removing inputs and outputs from reconciler structs:

• This is slightly smaller than [WIP] Remove pipelineResources in a Single Attempt #6000, could be an approach after 3 is done.

3. Remove resources and respective tests first:

• [TEP074] Remove CloudEvent pipelineResources #5995
• [TEP074] Remove Cluster pipelineResources #5996
• This would remove the test cases in reconciler first and breaks down the PR which contains them in [WIP] Remove pipelineResources in a Single Attempt #6000

I personally tend to move forward with the last approach as it is more efficient and easier to rollback.

[lbernick]

The last approach SGTM. Thanks for breaking this into small changes!

[JeromeJu]

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 #6002 is that seems the dependencies is at #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?

[JeromeJu]

Can we please tag this to v0.45 milestones as it shall target the next release? Thanks @tektoncd/core-maintainers

[JeromeJu]

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 Image resources 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 #6002 as the last pipelineResource type along with the removal of the generic pipelineResources functions?

[JeromeJu]

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 resources at 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 [TEP074] Remove Storage pipelineResources #6014 (comment), 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 [TEP074] Remove Storage, Git and Generic PipelineResources #6150

[JeromeJu]

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 #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 artifacts package?
  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.

[JeromeJu]

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

[JeromeJu]

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

[QuanZhang-William]

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.

[vdemeester]

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 🙃

[JeromeJu]

Thanks @vdemeester and the offline discussion with @QuanZhang-William .
Opened #6197 as a subtask of the current epic.

[pritidesai]

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

[lbernick]

Closing since PipelineResources have been fully removed!