-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[TEP0100] Remove EmbeddedStatus
Feature Flag
#6049
Conversation
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
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 |
700b283
to
419a1ef
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
1b873a4
to
f2abce1
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
f2abce1
to
5980bc4
Compare
The following is the coverage report on the affected files.
|
For the unit test
So I would like to confirm that this is the case and I can just remove the Condition check for |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
44f5d4f
to
f980406
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
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`.
f980406
to
b6090b3
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
@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:) |
There was a problem hiding this 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?
[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 |
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. |
There was a problem hiding this 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
/test pull-tekton-pipeline-beta-integration-tests |
/retest |
/retest |
Thanks Jerop, raised tektoncd/plumbing#1339 for followup at plumbing repo. |
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
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
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
andboth
EmbeddedStatus
andonly kept the
minimal
test cases as default.In detail, the test cases that checks the
taskruns
orruns
status fromprevious was changed to checking referenced
taskruns
orruns
fromthe
ChildReferences
.Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes