Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TEP0100] Remove EmbeddedStatus Feature Flag #6049

Merged

Conversation

JeromeJu
Copy link
Member

@JeromeJu JeromeJu commented Jan 26, 2023

Changes

This commit removes the embedded-status flag following up on the
Default to Minimal EmbeddedStatus in the v0.44 release. It removes all
the test cases related with the full and both EmbeddedStatus and
only kept the minimal test cases as default.

In detail, the test cases that checks the taskruns or runs status from
previous was changed to checking referenced taskruns or runs from
the ChildReferences.

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

`embedded-status` feature flag is removed. TaskRun and Run status will be populated only in `pipelineRun.status.childReferences`.

@tekton-robot tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 26, 2023
@tekton-robot tekton-robot added kind/misc Categorizes issue or PR as a miscellaneuous one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. labels Jan 26, 2023
@tekton-robot
Copy link
Collaborator

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/config/feature_flags.go 87.3% 86.0% -1.2
pkg/apis/pipeline/v1/pipeline_types.go 97.2% 97.2% -0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go 96.8% 96.7% -0.0
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go 94.6% 67.0% -27.6
pkg/reconciler/pipelinerun/cancel.go 90.4% 89.0% -1.3
pkg/reconciler/pipelinerun/pipelinerun.go 86.6% 87.3% 0.7

@tekton-robot
Copy link
Collaborator

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/config/feature_flags.go 87.3% 86.0% -1.2
pkg/apis/pipeline/v1/pipeline_types.go 97.2% 97.2% -0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go 96.8% 96.7% -0.0
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go 94.6% 67.0% -27.6
pkg/reconciler/pipelinerun/cancel.go 90.4% 89.0% -1.3
pkg/reconciler/pipelinerun/pipelinerun.go 86.6% 87.3% 0.7

@tekton-robot
Copy link
Collaborator

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/config/feature_flags.go 87.3% 86.0% -1.2
pkg/apis/pipeline/v1/pipeline_types.go 97.2% 97.2% -0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go 96.8% 96.7% -0.0
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go 94.6% 95.7% 1.1
pkg/reconciler/pipelinerun/cancel.go 90.4% 89.0% -1.3
pkg/reconciler/pipelinerun/pipelinerun.go 86.6% 87.3% 0.7

@tekton-robot
Copy link
Collaborator

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/config/feature_flags.go 87.3% 86.0% -1.2
pkg/apis/pipeline/v1/pipeline_types.go 97.2% 97.2% -0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go 96.8% 96.7% -0.0
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go 94.6% 95.7% 1.1
pkg/reconciler/pipelinerun/cancel.go 90.4% 89.0% -1.3
pkg/reconciler/pipelinerun/pipelinerun.go 86.6% 87.3% 0.7

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2023
@lbernick
Copy link
Member

Put up to decide if the ones for the plumbing repo should go in first.

There's no need to change anything in plumbing afaik. Clients should be able to upgrade to the version of pipelines with these changes without taking any additional steps, unless for some reason they rely on pipelinerun.status.taskruns.

@JeromeJu JeromeJu force-pushed the tep100-migrate-pipelinerunstatus branch 2 times, most recently from 700b283 to 419a1ef Compare January 27, 2023 22:16
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2023
@tekton-robot
Copy link
Collaborator

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/config/feature_flags.go 87.3% 86.0% -1.2
pkg/apis/pipeline/v1/pipeline_types.go 97.2% 97.2% -0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go 96.8% 96.7% -0.0
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go 94.6% 95.7% 1.1
pkg/reconciler/pipelinerun/cancel.go 90.4% 89.0% -1.3
pkg/reconciler/pipelinerun/pipelinerun.go 86.6% 87.3% 0.7

@tekton-robot
Copy link
Collaborator

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/config/feature_flags.go 87.3% 86.0% -1.2
pkg/apis/pipeline/v1/pipeline_types.go 97.2% 97.2% -0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go 96.8% 96.7% -0.0
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go 94.6% 95.7% 1.1
pkg/reconciler/pipelinerun/cancel.go 90.4% 89.0% -1.3
pkg/reconciler/pipelinerun/pipelinerun.go 86.6% 87.3% 0.7

@tekton-robot
Copy link
Collaborator

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/config/feature_flags.go 87.3% 86.0% -1.2
pkg/apis/pipeline/v1/pipeline_types.go 97.2% 97.2% -0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go 96.8% 96.7% -0.0
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go 94.6% 95.7% 1.1
pkg/reconciler/pipelinerun/cancel.go 90.4% 89.0% -1.3
pkg/reconciler/pipelinerun/pipelinerun.go 86.6% 87.3% 0.7

@tekton-robot
Copy link
Collaborator

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/config/feature_flags.go 87.3% 86.0% -1.2
pkg/apis/pipeline/v1/pipeline_types.go 97.2% 97.2% -0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go 96.8% 96.7% -0.0
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go 94.6% 95.7% 1.1
pkg/reconciler/pipelinerun/cancel.go 90.4% 89.0% -1.3
pkg/reconciler/pipelinerun/pipelinerun.go 86.6% 87.3% 0.7

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2023
@JeromeJu JeromeJu force-pushed the tep100-migrate-pipelinerunstatus branch from 1b873a4 to f2abce1 Compare January 30, 2023 23:42
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2023
@tekton-robot
Copy link
Collaborator

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/config/feature_flags.go 89.5% 88.6% -0.9
pkg/apis/pipeline/v1/pipeline_types.go 97.2% 97.2% -0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go 96.8% 96.7% -0.0
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go 94.6% 95.7% 1.1
pkg/reconciler/pipelinerun/cancel.go 90.4% 89.0% -1.3
pkg/reconciler/pipelinerun/pipelinerun.go 86.6% 87.3% 0.7

@tekton-robot
Copy link
Collaborator

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/config/feature_flags.go 89.5% 88.6% -0.9
pkg/apis/pipeline/v1/pipeline_types.go 97.2% 97.2% -0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go 96.8% 96.7% -0.0
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go 94.6% 95.7% 1.1
pkg/reconciler/pipelinerun/cancel.go 90.4% 89.0% -1.3
pkg/reconciler/pipelinerun/pipelinerun.go 86.6% 87.3% 0.7

@JeromeJu JeromeJu force-pushed the tep100-migrate-pipelinerunstatus branch from f2abce1 to 5980bc4 Compare January 31, 2023 14:18
@tekton-robot
Copy link
Collaborator

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/config/feature_flags.go 89.5% 88.6% -0.9
pkg/apis/pipeline/v1/pipeline_types.go 97.2% 97.2% -0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go 96.8% 96.7% -0.0
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go 94.6% 95.7% 1.1
pkg/reconciler/pipelinerun/cancel.go 90.4% 89.0% -1.3
pkg/reconciler/pipelinerun/pipelinerun.go 86.6% 85.2% -1.4

@JeromeJu
Copy link
Member Author

For the unit test TestReconcilePipeline_FinalTasks, I tried to use the queried taskruns in pipelinerun.status.childreferences to assert but found when we deleted the updateTaskRunsStatusDirectly, the Conditions will not be updated, which corresponds to the TEP100, since it has been deprecated:

This struct, and ChildStatusReferences.ConditionChecks, will be removed once Conditions, which have been deprecated, are removed completely. We are not using child references for the conditions' statuses, because ConditionCheckStatus, the only thing in PipelineRunConditionCheckStatus other than the ConditionName, isn't replicated anywhere else, and contains a fairly minimal amount of data - the pod name, start and completion times, and a corev1.ContainerState. See [the issue for deprecating Conditions](https://github.com/tektoncd/community/blob/main/teps/issue-3377) for more information on the planned removal of Conditions.

So I would like to confirm that this is the case and I can just remove the Condition check for taskruns status in the first 3 test cases of TestReconcilePipeline_FinalTasks?

@tekton-robot
Copy link
Collaborator

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/config/feature_flags.go 89.5% 88.6% -0.9
pkg/apis/pipeline/v1/pipeline_types.go 97.2% 97.2% -0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go 96.8% 96.7% -0.0
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go 94.6% 95.7% 1.1
pkg/reconciler/pipelinerun/cancel.go 90.4% 90.4% 0.0
pkg/reconciler/pipelinerun/pipelinerun.go 86.6% 87.3% 0.7
pkg/status/status.go 86.8% 84.9% -1.9

@tekton-robot
Copy link
Collaborator

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/config/feature_flags.go 89.5% 88.6% -0.9
pkg/apis/pipeline/v1/pipeline_types.go 97.2% 97.2% -0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go 96.8% 96.7% -0.0
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go 94.6% 95.7% 1.1
pkg/reconciler/pipelinerun/cancel.go 90.4% 90.4% 0.0
pkg/reconciler/pipelinerun/pipelinerun.go 86.6% 87.3% 0.7
pkg/status/status.go 86.8% 84.9% -1.9

@JeromeJu JeromeJu force-pushed the tep100-migrate-pipelinerunstatus branch 2 times, most recently from 44f5d4f to f980406 Compare February 2, 2023 16:26
@tekton-robot
Copy link
Collaborator

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/config/feature_flags.go 89.5% 88.6% -0.9
pkg/apis/pipeline/v1/pipeline_types.go 97.2% 97.2% -0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go 96.8% 96.7% -0.0
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go 94.6% 95.7% 1.1
pkg/reconciler/pipelinerun/cancel.go 90.4% 90.4% 0.0
pkg/reconciler/pipelinerun/pipelinerun.go 86.6% 87.1% 0.5
pkg/status/status.go 86.8% 84.9% -1.9

@tekton-robot
Copy link
Collaborator

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/config/feature_flags.go 89.5% 88.6% -0.9
pkg/apis/pipeline/v1/pipeline_types.go 97.2% 97.2% -0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go 96.8% 96.7% -0.0
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go 94.6% 95.7% 1.1
pkg/reconciler/pipelinerun/cancel.go 90.4% 90.4% 0.0
pkg/reconciler/pipelinerun/pipelinerun.go 86.6% 87.2% 0.6
pkg/status/status.go 86.8% 84.9% -1.9

@tekton-robot
Copy link
Collaborator

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/config/feature_flags.go 89.5% 88.6% -0.9
pkg/apis/pipeline/v1/pipeline_types.go 97.2% 97.2% -0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go 96.8% 96.7% -0.0
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go 94.6% 95.7% 1.1
pkg/reconciler/pipelinerun/cancel.go 90.4% 90.4% 0.0
pkg/reconciler/pipelinerun/pipelinerun.go 86.6% 87.2% 0.6
pkg/status/status.go 86.8% 84.9% -1.9

This commit removes the `EmbeddedStatus` feature flag. It removes all
the test cases related with the `full` and `both` `EmbeddedStatus` and
only kept the `minimal` test cases as default.

In detail, the test cases that checks the `taskruns` or `runs` status from
previous was changed to checking referenced `taskruns` or `runs` from
the `ChildReferences`.
@JeromeJu JeromeJu force-pushed the tep100-migrate-pipelinerunstatus branch from f980406 to b6090b3 Compare February 2, 2023 18:47
@tekton-robot
Copy link
Collaborator

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/config/feature_flags.go 89.5% 88.6% -0.9
pkg/apis/pipeline/v1/pipeline_types.go 97.2% 97.2% -0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go 96.8% 96.7% -0.0
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go 94.6% 95.7% 1.1
pkg/reconciler/pipelinerun/cancel.go 90.4% 90.4% 0.0
pkg/reconciler/pipelinerun/pipelinerun.go 86.6% 87.2% 0.6
pkg/status/status.go 86.8% 84.9% -1.9

@tekton-robot
Copy link
Collaborator

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/config/feature_flags.go 89.5% 88.6% -0.9
pkg/apis/pipeline/v1/pipeline_types.go 97.2% 97.2% -0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go 96.8% 96.7% -0.0
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go 94.6% 95.7% 1.1
pkg/reconciler/pipelinerun/cancel.go 90.4% 90.4% 0.0
pkg/reconciler/pipelinerun/pipelinerun.go 86.6% 87.2% 0.6
pkg/status/status.go 86.8% 84.9% -1.9

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2023
@lbernick
Copy link
Member

lbernick commented Feb 3, 2023

@JeromeJu just edited the release notes since we don't really have a way for people to migrate off of the feature flag; they just can't use it anymore

@JeromeJu
Copy link
Member Author

JeromeJu commented Feb 3, 2023

@JeromeJu just edited the release notes since we don't really have a way for people to migrate off of the feature flag; they just can't use it anymore

Ohhh thanks for correcting that:)

Copy link
Member

@chitrangpatel chitrangpatel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just wanted to ask what happens if users provide embedded-status flag. Do they get an error or does it just silently ignore it?

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@JeromeJu
Copy link
Member Author

JeromeJu commented Feb 3, 2023

Looks good! Just wanted to ask what happens if users provide embedded-status flag. Do they get an error or does it just silently ignore it?

I don't think they will get an error as it shall be just an unknown feature flag in config/config-feature-flag

@chitrangpatel
Copy link
Member

Looks good! Just wanted to ask what happens if users provide embedded-status flag. Do they get an error or does it just silently ignore it?

I don't think they will get an error as it shall be just an unknown feature flag in config/config-feature-flag

I think thats ok.

Copy link
Member

@jerop jerop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JeromeJu please also need to remove embedded-status feature flag from dogfooding - https://github.com/tektoncd/plumbing/blob/main/tekton/cd/pipeline/overlays/dogfooding/feature-flags.yaml#L8

thank you!

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2023
@JeromeJu
Copy link
Member Author

JeromeJu commented Feb 3, 2023

/test pull-tekton-pipeline-beta-integration-tests

@JeromeJu
Copy link
Member Author

JeromeJu commented Feb 3, 2023

/retest
to retrigger ci for build test

@JeromeJu
Copy link
Member Author

JeromeJu commented Feb 3, 2023

/retest

@JeromeJu
Copy link
Member Author

JeromeJu commented Feb 3, 2023

Thanks Jerop, raised tektoncd/plumbing#1339 for followup at plumbing repo.

@tekton-robot tekton-robot merged commit d2f075a into tektoncd:main Feb 3, 2023
zregvart added a commit to zregvart/tkn-cli that referenced this pull request Apr 12, 2023
The `EmbeddedStatus` feature flag was removed in[1], with that the
`TaskRuns` field is no longer available in the `v1beta1.PipelineRun`'s
`Status`. This adapts the test utilizing that field and allows the
upgrade to Tekton Pipeline v0.45.0.

The `PipelineRunHasFailed` function in `test/builder/validate.go`
doesn't seem to be used, so I removed it rather than adapting it to use
`ChildReferences`.

I've tested this by running `make` with Tekton Pipeline at v0.44.0 and
at v0.45.0.

[1] tektoncd/pipeline#6049
tekton-robot pushed a commit to tektoncd/cli that referenced this pull request Apr 13, 2023
The `EmbeddedStatus` feature flag was removed in[1], with that the
`TaskRuns` field is no longer available in the `v1beta1.PipelineRun`'s
`Status`. This adapts the test utilizing that field and allows the
upgrade to Tekton Pipeline v0.45.0.

The `PipelineRunHasFailed` function in `test/builder/validate.go`
doesn't seem to be used, so I removed it rather than adapting it to use
`ChildReferences`.

I've tested this by running `make` with Tekton Pipeline at v0.44.0 and
at v0.45.0.

[1] tektoncd/pipeline#6049
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/misc Categorizes issue or PR as a miscellaneuous one. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants