[TEP074] Tombstone ResourceResult field with the removal of PipelineResources

[JeromeJu]

Changes

This commit tombstones the resourcesResult field of TaskRun along with the removal of pipelineResources.
It aims to cleanup pkg/pod/status as the prerequisites swapping to v1 as storage version.

It is tombstoned due to following reasons:

• ResourceResult, according its description, was 'Results from Resources built during the TaskRun. currently includes the digest of build container images', now since image resources are removed, it was left meaningless.
• ResourceResult highly relies on PipelineResourcesResult type, where previously PipelineResourceResult is being written to it, now that PipelineResources is being removed, there is no usage for resourcesResult
• ResourceResult has not been moved to v1, when we swap the storage versions, there will not be a suitable place to write to PipelineResourceResult any more

The migration of PipelineResourceResult is broken down into:

• Stop populating resourceName in git-init image Stop populating resourceName in git-init image #6310
• removing PipelineResourceResultType of PipelineResourceResult Struct ([TEP074] Remove PipelineResourceResultType of PipelineResourceResult Struct #6198)
• tombstoning of deprecated field resourcesResult ([TEP074] Tombstone ResourceResult field with the removal of PipelineResources #6301)

/kind misc
part of #6197

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

• [n/a] Has Docs included if any changes are user facing
• Has Tests included if any functionality added or changed
• Follows the commit message standard
• Meets the Tekton contributor standards (including
  functionality, content, code)
• 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
• Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
• Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

[tekton-robot]

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/apis/pipeline/v1beta1/taskrun_conversion.go	92.3%	93.1%	0.8
pkg/pod/status.go	90.9%	91.2%	0.4

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/taskrun_conversion.go	92.3%	93.1%	0.8
pkg/pod/status.go	90.9%	91.2%	0.4

[tekton-robot]

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/apis/pipeline/v1beta1/taskrun_conversion.go	92.3%	93.1%	0.8
pkg/pod/status.go	90.9%	91.2%	0.4

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/taskrun_conversion.go	92.3%	93.1%	0.8
pkg/pod/status.go	90.9%	91.2%	0.4

[afrittoli]

This makes sense since we don't have PipelineResources anymore.
I'm slightly concerned that we did not explicitly deprecate this field, so the removal might break users.
However, since we deprecated PipelineResources a long time ago though, and this field is only populated by PipelineResources, it feels like we gave quite some notice already.

To be on the safe side, we could mark it as deprecated in the code, and remove it in the following release?
@tektoncd/core-maintainers opinions?

[abayer]

Based on past cases where we’ve removed fields in the status and the pain that’s caused with downstream consumers of the status structs, I’d be conservative with the removal.

[JeromeJu]

Based on past cases where we’ve removed fields in the status and the pain that’s caused with downstream consumers of the status structs, I’d be conservative with the removal.

Thanks @abayer , I agree with the cautions that we need to take when dealing with api fields. But when I tried removing ResourcesResult, it didn't feel like much functionalities related apart from the PipelineResourceResult writing to it. Would it be reasonable that we tombstone this field in the current release whilst remove the usage in pkg/pod/status and then we remove the field in the following release? Or I would love to have advice/plans if there are other precautions to be taken 🙏

[JeromeJu]

This makes sense since we don't have PipelineResources anymore. I'm slightly concerned that we did not explicitly deprecate this field, so the removal might break users. However, since we deprecated PipelineResources a long time ago though, and this field is only populated by PipelineResources, it feels like we gave quite some notice already.

To be on the safe side, we could mark it as deprecated in the code, and remove it in the following release? @tektoncd/core-maintainers opinions?

Appreciate the help @afrittoli , I also added the reasons why this was attempted to be removed: (also in the PR comment)

• ResourceResult, according its description, was 'Results from Resources built during the TaskRun. currently includes the digest of build container images', now since image resources are removed, it was left meaningless.
• ResourceResult highly relies on PipelineResourcesResult type, where previously PipelineResourceResult is being written to it, now that PipelineResources is being removed, there is no usage for resourcesResult
• ResourceResult has not been moved to v1, when we swap the storage versions, there will not be a suitable place to write to PipelineResourceResult any more

[tekton-robot]

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/apis/pipeline/v1beta1/resource_types.go	80.0%	85.7%	5.7
pkg/pod/status.go	90.9%	90.8%	-0.0

[JeromeJu]

/unhold
#6434 is merged.

[tekton-robot]

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/apis/pipeline/v1beta1/taskrun_conversion.go	93.1%	94.1%	0.9
pkg/pod/status.go	90.9%	91.2%	0.3

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/taskrun_conversion.go	93.1%	94.1%	0.9
pkg/pod/status.go	90.9%	91.2%	0.3

[tekton-robot]

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/apis/pipeline/v1beta1/taskrun_conversion.go	93.1%	94.1%	0.9
pkg/pod/status.go	90.9%	91.2%	0.3

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/taskrun_conversion.go	93.1%	94.1%	0.9
pkg/pod/status.go	90.9%	91.2%	0.3

[tekton-robot]

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/apis/pipeline/v1beta1/taskrun_conversion.go	93.1%	94.1%	0.9
pkg/pod/status.go	90.9%	91.2%	0.3

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/taskrun_conversion.go	93.1%	94.1%	0.9
pkg/pod/status.go	90.9%	91.2%	0.3

[dibyom]

@JeromeJu I don't think this needs an action-required release note since there is nothing actionable for the Tekton user. This is an internal cleanup part of the pipeline resources removal, and pipeline resources have already been removed.

[JeromeJu]

This is an internal cleanup part of the pipeline resources removal, and pipeline resources have already been removed.

Thanks Dibyo, I agree with that the TaskRunStatus could not be specified, so this is just not populating the field. Removed the release notes.

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernick, vdemeester

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:

• OWNERS [lbernick,vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[lbernick]

/lgtm