Revert removal of embedded TaskRun status in API. #6206

wlynch · 2023-02-22T14:09:32Z

Changes

This is a partial revert to undo the removal of the type from the API. Because minimal statuses were only being populated by default in v0.44.0, the removal of this field in v0.45.0 makes it difficult for clients >= v0.45 to safely communicate with older server versions (in particular LTS releases).

Only the API type is needed - newer server versions do not need to populate or support it.

Fixes tektoncd/chains#715

/kind bug

Submitter Checklist

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

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

PipelineRun Embedded TaskRun statuses reintroduced as deprecated to allow compatibility with older server versions.

wlynch · 2023-02-22T14:10:09Z

/cc @JeromeJu @lbernick

tekton-robot · 2023-02-22T14:10:12Z

@wlynch: GitHub didn't allow me to request PR reviews from the following users: JeromeJu.

Note that only tektoncd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @JeromeJu @lbernick

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.

wlynch · 2023-02-22T14:11:17Z

/kind bug

lbernick

a few q's:

I'm assuming we'll backport to v0.45?
How long do you think we need to keep this in place for?
This seems like it would apply to removal of any fields (and we are about to remove PipelineResources); can we create general guidance for how to remove fields? (not a blocker for this PR)

pkg/apis/pipeline/v1beta1/pipelinerun_types.go

wlynch · 2023-02-22T14:50:14Z

I'm assuming we'll backport to v0.45

Yes please, though this only requires a client library release (e.g. you can just cut a tag). This has no impact on the server images.

How long do you think we need to keep this in place for?

Depends. At minimum I'd keep this in place until the current LTS releases that default to full statuses cycle out (which I think is only v0.41.0). If you want to be extra conservative this should stay in place until v0.44.0 cycles out (since users could still opt back into full statuses).

Since this is only preserving an API field, my hope is the maintenance burden for this is very low.

This seems like it would apply to removal of any fields (and we are about to remove PipelineResources); can we create general guidance for how to remove fields? (not a blocker for this PR)

I can kick this off in another PR!

lbernick · 2023-02-22T15:09:18Z

How long do you think we need to keep this in place for?

Depends. At minimum I'd keep this in place until the current LTS releases that default to full statuses cycle out (which I think is only v0.41.0). If you want to be extra conservative this should stay in place until v0.44.0 cycles out (since users could still opt back into full statuses).

That makes sense; I think v0.44 would make sense since we'd want to support until it's not possible to opt into full statuses. Can you add a comment to the code about when this can be removed?

Since this is only preserving an API field, my hope is the maintenance burden for this is very low.

Yeah that would be my expectation as well

This seems like it would apply to removal of any fields (and we are about to remove PipelineResources); can we create general guidance for how to remove fields? (not a blocker for this PR)

I can kick this off in another PR!

thank you!

JeromeJu · 2023-02-22T15:11:59Z

This seems like it would apply to removal of any fields (and we are about to remove PipelineResources); can we create general guidance for how to remove fields? (not a blocker for this PR)
I can kick this off in another PR!

Thanks @wlynch , I would assume we might also need to mention this in the deprecation.md against f3e9fc1

wlynch · 2023-02-22T15:55:52Z

Thanks @wlynch , I would assume we might also need to mention this in the deprecation.md against f3e9fc1

This is an interesting question 🤔 My initial thought is that this is no longer a server feature, so we don't necessarily need to include it here anymore - it just needs to be documented in the client.

I don't have strong feelings about this though - @tektoncd/core-maintainers thoughts?

JeromeJu · 2023-02-22T16:01:29Z

pkg/client/clientset/versioned/scheme/register.go

-//     clientsetscheme "k8s.io/client-go/kubernetes/scheme"
-//     aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme"
-//   )
+//	import (


This might need to be restored

This was all from ./hack/update-codegen.sh 🤷

vdemeester · 2023-02-22T16:41:45Z

Thanks @wlynch , I would assume we might also need to mention this in the deprecation.md against f3e9fc1

This is an interesting question thinking My initial thought is that this is no longer a server feature, so we don't necessarily need to include it here anymore - it just needs to be documented in the client.

I don't have strong feelings about this though - @tektoncd/core-maintainers thoughts?

We might want to have something similar for "go types".. but then, if the deprecated function already have the correct "annotation" it should shows in godoc, ide(s), …

pritidesai · 2023-02-22T16:45:10Z

Thanks @wlynch , I would assume we might also need to mention this in the deprecation.md against f3e9fc1

This is an interesting question 🤔 My initial thought is that this is no longer a server feature, so we don't necessarily need to include it here anymore - it just needs to be documented in the client.

I don't have strong feelings about this though - @tektoncd/core-maintainers thoughts?

I would have never deleted this entry in the deprecations.md:

| [The `PipelineRun.Status.TaskRuns` and `PipelineRun.Status.Runs` fields; the `full` and `both` `embedded-status` values along with their functionalities are deprecated and will be removed in v0.45.](https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md) | v0.35.0 | Beta | Jan 25, 2023 |

The reasons being:

The pipelines project is constantly moving but the users are not constantly adopting those changes. It is not possible in practice to adopt them on a regular basis. It always helps to track how we landed to a certain decision. Keeping the history falls in the bucket of anything we can do to help the operators/consumers/users to design their workflows, pipelines, and API.

JeromeJu · 2023-02-22T17:00:01Z

I would have never deleted this entry in the deprecations.md:
| [The `PipelineRun.Status.TaskRuns` and `PipelineRun.Status.Runs` fields; the `full` and `both` `embedded-status` values along with their functionalities are deprecated and will be removed in v0.45.](https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md) | v0.35.0 | Beta | Jan 25, 2023 |
The reasons being:

The pipelines project is constantly moving but the users are not constantly adopting those changes. It is not possible in practice to adopt them on a regular basis. It always helps to track how we landed to a certain decision. Keeping the history falls in the bucket of anything we can do to help the operators/consumers/users to design their workflows, pipelines, and API.

That makes sense for helping users to keep in track.

In that case, shall we have a separate table representing the 'removed' or 'already deprecated' APIs in deprecation.md?
If so I am happy to bring up #6209 if that's helpful.

wlynch · 2023-03-07T23:27:14Z

Deprecation documentation is covered by #6209 - any other blockers for this PR?

vdemeester · 2023-03-08T13:00:10Z

@wlynch I don't think so.
/approve

tekton-robot · 2023-03-08T13:00:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [vdemeester]

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

lbernick · 2023-03-08T14:26:44Z

/lgtm

jerop · 2023-03-08T14:39:44Z

/hold

@wlynch could you please squash the commits?

vdemeester · 2023-03-09T16:05:47Z

@wlynch seems like you also need to rebase you PR 🤔

This is a partial revert to undo the removal of the type from the API. Because minimal statuses were only being populated by default in v0.44.0, the removal of this field in v0.45.0 makes it difficult for clients >= v0.45 to safely communicate with older server versions (in particular LTS releases). Only the API type is needed - newer server versions do not need to populate or support it.

wlynch · 2023-03-14T14:24:55Z

Done.

/remove-hold

wlynch · 2023-03-14T14:25:27Z

/assign @vdemeester

wlynch · 2023-03-16T00:06:16Z

/milestone Pipelines v0.46

I'd like to track this as part of the v0.46 milestone so another release doesn't ship without this.

wlynch · 2023-03-16T00:08:34Z

@JeromeJu Looks like Lee's out - can I get an LGTM? 🙏

lbernick · 2023-03-16T00:09:38Z

/lgtm

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 22, 2023

tekton-robot requested review from imjasonh and jerop February 22, 2023 14:09

tekton-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 22, 2023

tekton-robot requested a review from lbernick February 22, 2023 14:10

tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Feb 22, 2023

lbernick reviewed Feb 22, 2023

View reviewed changes

pkg/apis/pipeline/v1beta1/pipelinerun_types.go Show resolved Hide resolved

tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 22, 2023

wlynch requested a review from lbernick February 22, 2023 14:57

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 22, 2023

JeromeJu reviewed Feb 22, 2023

View reviewed changes

wlynch mentioned this pull request Feb 23, 2023

Bump tektoncd/pipelines dependency to v0.45 tektoncd/chains#712

Merged

6 tasks

wlynch force-pushed the pr-embeddedstatus-revert branch 2 times, most recently from 53bc736 to 4124ed6 Compare February 24, 2023 17:23

JeromeJu mentioned this pull request Mar 3, 2023

Create Removed Table for deprecation.md #6209

Merged

5 tasks

wlynch requested a review from JeromeJu March 7, 2023 23:27

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

tekton-robot assigned lbernick Mar 8, 2023

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

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2023

wlynch force-pushed the pr-embeddedstatus-revert branch from 4124ed6 to 8a21576 Compare March 14, 2023 14:24

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

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 14, 2023

tekton-robot assigned vdemeester Mar 14, 2023

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

tekton-robot merged commit 089efd8 into tektoncd:main Mar 16, 2023

This was referenced Mar 23, 2023

[TEP074] Remove Storage, Git and Generic PipelineResources #6150

Merged

Manually bump tektoncd/pipelines dependency to v0.45 tektoncd/chains#711

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert removal of embedded TaskRun status in API. #6206

Revert removal of embedded TaskRun status in API. #6206

wlynch commented Feb 22, 2023 •

edited

Loading

wlynch commented Feb 22, 2023

tekton-robot commented Feb 22, 2023

wlynch commented Feb 22, 2023

lbernick left a comment

wlynch commented Feb 22, 2023

lbernick commented Feb 22, 2023 •

edited

Loading

JeromeJu commented Feb 22, 2023 •

edited

Loading

wlynch commented Feb 22, 2023

JeromeJu Feb 22, 2023

wlynch Feb 22, 2023

lbernick Feb 22, 2023

vdemeester commented Feb 22, 2023

pritidesai commented Feb 22, 2023 •

edited

Loading

JeromeJu commented Feb 22, 2023 •

edited

Loading

wlynch commented Mar 7, 2023

vdemeester commented Mar 8, 2023

tekton-robot commented Mar 8, 2023

lbernick commented Mar 8, 2023

jerop commented Mar 8, 2023

vdemeester commented Mar 9, 2023

wlynch commented Mar 14, 2023

wlynch commented Mar 14, 2023

wlynch commented Mar 16, 2023

wlynch commented Mar 16, 2023

lbernick commented Mar 16, 2023

Revert removal of embedded TaskRun status in API. #6206

Revert removal of embedded TaskRun status in API. #6206

Conversation

wlynch commented Feb 22, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

wlynch commented Feb 22, 2023

tekton-robot commented Feb 22, 2023

wlynch commented Feb 22, 2023

lbernick left a comment

Choose a reason for hiding this comment

wlynch commented Feb 22, 2023

lbernick commented Feb 22, 2023 • edited Loading

JeromeJu commented Feb 22, 2023 • edited Loading

wlynch commented Feb 22, 2023

JeromeJu Feb 22, 2023

Choose a reason for hiding this comment

wlynch Feb 22, 2023

Choose a reason for hiding this comment

lbernick Feb 22, 2023

Choose a reason for hiding this comment

vdemeester commented Feb 22, 2023

pritidesai commented Feb 22, 2023 • edited Loading

JeromeJu commented Feb 22, 2023 • edited Loading

wlynch commented Mar 7, 2023

vdemeester commented Mar 8, 2023

tekton-robot commented Mar 8, 2023

lbernick commented Mar 8, 2023

jerop commented Mar 8, 2023

vdemeester commented Mar 9, 2023

wlynch commented Mar 14, 2023

wlynch commented Mar 14, 2023

wlynch commented Mar 16, 2023

wlynch commented Mar 16, 2023

lbernick commented Mar 16, 2023

wlynch commented Feb 22, 2023 •

edited

Loading

lbernick commented Feb 22, 2023 •

edited

Loading

JeromeJu commented Feb 22, 2023 •

edited

Loading

pritidesai commented Feb 22, 2023 •

edited

Loading

JeromeJu commented Feb 22, 2023 •

edited

Loading