Stop populating resourceName in git-init image #6310

lbernick · 2023-03-07T16:38:06Z

The git-init image writes a git commit SHA and URL to a pod's termination message. Prior to this commit, if these values came from the output of a PipelineResource, it would also write the name of that PipelineResource to the pod's termination message.

Since PipelineResources have been removed, the environment variable TEKTON_RESOURCE_NAME is never populated, and the PipelineResource name is never written to termination messages. This commit removes the check for this environment variable and stops writing this value to the termination message. The termination message will still be parseable by other code.

Removing this functionality will make it easier to migrate to v1, by allowing us to remove fields not relevant to our v1 API. Refactoring/renaming PipelineResourceResult will happen in a separate commit.

Part of #6197

/kind cleanup

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
n/a 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
n/a Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
n/a 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 · 2023-03-07T16:38:08Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

lbernick · 2023-03-07T16:38:43Z

Will need to wait until #6150 is merged.

JeromeJu

Thanks @lbernick 😃
#6150 has been merged we can also get this in:)

Yongxuanzhang

/lgtm
Thanks!

wlynch

I vaguely remember this being used by the termination message decoder to distinguish resource outputs from param outputs. Would the removal of this cause conflicts if a param happened to share the same name? 🤔

If we're completely removing the Git pipeline resource, we might be able to rip out git-init entirely starting from the controller. AFAICT from code search this is only used in examples, and I'm pretty sure that was mostly done from convenience.

There might be some catalog usage copied from the examples, but I think we should push the maintenance burden onto the catalog tasks if they want to continue using it.

lbernick · 2023-03-08T14:40:39Z

I vaguely remember this being used by the termination message decoder to distinguish resource outputs from param outputs. Would the removal of this cause conflicts if a param happened to share the same name? 🤔

If we're completely removing the Git pipeline resource, we might be able to rip out git-init entirely starting from the controller. AFAICT from code search this is only used in examples, and I'm pretty sure that was mostly done from convenience.

There might be some catalog usage copied from the examples, but I think we should push the maintenance burden onto the catalog tasks if they want to continue using it.

The git PipelineResource was removed in #6150. git-init is still going to be used by two catalog tasks that will be moved to the verified catalog, and this code is going to be pulled out of the pipelines repo into the tektoncd-catalog org (https://github.com/tektoncd/community/blob/main/teps/0074-deprecate-pipelineresources.md#images-used-in-pipelineresources). For now it's easier to just make this change to help unblock v1 related changes than it is to move the git-init code out.

/retest

The git-init image writes a git commit SHA and URL to a pod's termination message. Prior to this commit, if these values came from the output of a PipelineResource, it would also write the name of that PipelineResource to the pod's termination message. Since PipelineResources have been removed, the environment variable TEKTON_RESOURCE_NAME is never populated, and the PipelineResource name is never written to termination messages. This commit removes the check for this environment variable and stops writing this value to the termination message. The termination message will still be parseable by other code. Removing this functionality will make it easier to migrate to v1, by allowing us to remove fields not relevant to our v1 API. Refactoring/renaming PipelineResourceResult will happen in a separate commit.

tekton-robot · 2023-03-09T15:00:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JeromeJu, jerop

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~~ [jerop]

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

Yongxuanzhang · 2023-03-09T15:20:41Z

/lgtm

Yongxuanzhang · 2023-03-09T15:20:54Z

/retest

vdemeester · 2023-03-09T15:27:48Z

pkg/apis/pipeline/v1beta1/resource_types.go

+	Key   string `json:"key"`
+	Value string `json:"value"`
+	// ResourceName may be used in tests, but it is not populated in termination messages.
+	// It is preserved here for backwards compatibility and will not be ported to v1.


Do we want to mark it as deprecated ? // Deprecated: It is preserved …

I think we would want to rename the PipelineResoruceType to a new Struct as it is no longer meaningful without PipelineResources. and I could do the marking of deprecation while leaving the old Struct there in v1beta1 with backward compatibility if that sounds good?

also noted and tracked here at #6197 (comment)

tekton-robot requested review from bobcatfish and dibyom March 7, 2023 16:38

lbernick added this to the Pipelines v0.46 milestone Mar 7, 2023

JeromeJu mentioned this pull request Mar 7, 2023

PipelineResourceResult Renamed and Moving off resource_types #6197

Closed

4 tasks

JeromeJu approved these changes Mar 7, 2023

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 7, 2023

lbernick force-pushed the results branch from 9730699 to acbe11d Compare March 7, 2023 21:16

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 7, 2023

lbernick marked this pull request as ready for review March 7, 2023 21:16

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 7, 2023

tekton-robot requested review from afrittoli and wlynch March 7, 2023 21:17

Yongxuanzhang reviewed Mar 7, 2023

View reviewed changes

tekton-robot assigned Yongxuanzhang Mar 7, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2023

wlynch reviewed Mar 7, 2023

View reviewed changes

JeromeJu mentioned this pull request Mar 8, 2023

[TEP074] Tombstone ResourceResult field with the removal of PipelineResources #6301

Merged

6 tasks

lbernick force-pushed the results branch from acbe11d to a06cf2c Compare March 9, 2023 14:44

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 9, 2023

lbernick mentioned this pull request Mar 9, 2023

Migrate git-init image off PipelineResourceResult #6335

Closed

6 tasks

jerop approved these changes Mar 9, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 9, 2023

vdemeester reviewed Mar 9, 2023

View reviewed changes

tekton-robot merged commit 2a15755 into tektoncd:main Mar 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop populating resourceName in git-init image #6310

Stop populating resourceName in git-init image #6310

lbernick commented Mar 7, 2023

tekton-robot commented Mar 7, 2023

lbernick commented Mar 7, 2023 •

edited

Loading

JeromeJu left a comment •

edited

Loading

Yongxuanzhang left a comment

wlynch left a comment

lbernick commented Mar 8, 2023

tekton-robot commented Mar 9, 2023

Yongxuanzhang commented Mar 9, 2023

Yongxuanzhang commented Mar 9, 2023

vdemeester Mar 9, 2023

JeromeJu Mar 9, 2023 •

edited

Loading

JeromeJu Mar 9, 2023

Stop populating resourceName in git-init image #6310

Stop populating resourceName in git-init image #6310

Conversation

lbernick commented Mar 7, 2023

Submitter Checklist

Release Notes

tekton-robot commented Mar 7, 2023

lbernick commented Mar 7, 2023 • edited Loading

JeromeJu left a comment • edited Loading

Choose a reason for hiding this comment

Yongxuanzhang left a comment

Choose a reason for hiding this comment

wlynch left a comment

Choose a reason for hiding this comment

lbernick commented Mar 8, 2023

tekton-robot commented Mar 9, 2023

Yongxuanzhang commented Mar 9, 2023

Yongxuanzhang commented Mar 9, 2023

vdemeester Mar 9, 2023

Choose a reason for hiding this comment

JeromeJu Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

JeromeJu Mar 9, 2023

Choose a reason for hiding this comment

lbernick commented Mar 7, 2023 •

edited

Loading

JeromeJu left a comment •

edited

Loading

JeromeJu Mar 9, 2023 •

edited

Loading