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.

Sign up for GitHub

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

Jump to bottom

TEP-0100: PipelineRun Status Migration #4954

Closed
2 of 3 tasks
jerop opened this issue Jun 9, 2022 · 10 comments · Fixed by #6099
Closed
2 of 3 tasks

TEP-0100: PipelineRun Status Migration #4954

jerop opened this issue Jun 9, 2022 · 10 comments · Fixed by #6099
Assignees
@JeromeJu
Labels
area/api Indicates an issue or PR that deals with the API. area/epic Issues that should be considered as Epics (aka multiple sub-tasks, …) kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Milestone
Pipelines v0.45

Comments

@jerop
Copy link
Member

jerop commented Jun 9, 2022 •
edited by afrittoli
Loading

TEP-0100: Embedded TaskRuns and Runs Status in PipelineRuns proposed changes to PipelineRun status to reduce the amount of information stored about the status of TaskRuns and Runs to improve performance, reduce memory bloat and improve extensibility.

This feature was released in v0.35.0 gated behind the embedded-status feature flag, and this issue tracks the migration and removal of that feature flag.

  • v0.35.0: embedded-status flag defaults to full
  • v0.44.0: embedded-status flag defaults to minimal
  • v0.45.0: embedded-status flag is removed
@jerop jerop added kind/feature Categorizes issue or PR as related to a new feature. area/epic Issues that should be considered as Epics (aka multiple sub-tasks, …) labels Jun 9, 2022
@jerop jerop added this to the Pipelines v0.44 milestone Jun 9, 2022
@jerop jerop added the area/api Indicates an issue or PR that deals with the API. label Jun 9, 2022
This was referenced Jun 23, 2022
Closed
Merged
Closed
@jerop
Copy link
Member Author

jerop commented Jun 24, 2022

06.24 update: we are dogfooding the minimal status in the alpha integration tests

@tekton-robot
Copy link
Collaborator

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

@tekton-robot tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 22, 2022
@jerop
Copy link
Member Author

jerop commented Oct 12, 2022

/lifecycle frozen

@tekton-robot tekton-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 12, 2022
@jerop jerop mentioned this issue Dec 20, 2022
Closed
This was referenced Jan 12, 2023
Merged
Merged
Closed
Closed
@afrittoli
Copy link
Member

One part of this will be addressed in v0.44, but I think we will only close in v0.45, so perhaps we should move this to that milestone?

@JeromeJu
Copy link
Member

One part of this will be addressed in v0.44, but I think we will only close in v0.45, so perhaps we should move this to that milestone?

The TEP100 and this issue shall close only in v0.45, sounds good to me. Thanks @afrittoli

@JeromeJu
Copy link
Member

JeromeJu commented Jan 20, 2023 •
edited
Loading

  • v0.45.0: embedded-status flag is removed

    • For the changes in v0.45, the test cases with FullEmbeddedStatus shall be removed along with the flag value.
    • The pipelinerun reconciler needs to be updated to not to rely on cfg.featureflags.embeddedstatus
    • Remove the embedded-status feature flags in plumbing repo and decide the sequence.

@JeromeJu
Copy link
Member

JeromeJu commented Jan 26, 2023 •
edited
Loading

Updated to implement in a single PR

Earlier I drafted up #6049 only to figure out which would be the better practice for implementing the removal of the embedded-status flag for v0.45. And for the purpose of code hygiene and easy rollback for review, I would like to propose separating them into the following:

one pull request for refactoring/ removing all the test cases for pipelineRunStatus with full and both embedded status
one pull request for the integration test cases for removing the requireAnyGate of full or both embedded status
one pull request for changing the reconciler logics and removing the flag
one pull request for changing the embedded-status in the plumbing repo for prow setup

Please let me know if this sounds good cc @abayer @jerop @lbernick. Thanks!

@lbernick
Copy link
Member

tektoncd/community#931 is not merged yet, but in it, I added the guidance "Tests and documentation (where applicable) should always be part of the same pull request when introducing a change". To expand on this a bit in this context: If you remove tests first, we now have untested functionality (different values of the feature flag) in our code base. We wouldn't allow someone to introduce functionality without tests, so we should also not allow tests to be removed before the functionality is. The same is true for documentation. In terms of "easy rollback": if there was an issue with removing the feature flag and we wanted to revert the commit removing the flag, we would also want to revert removal of the tests. Therefore, I'd put this all in the same PR.

Changing our prow cluster configuration is a separate repo, so that can happen completely independently.

@JeromeJu
Copy link
Member

JeromeJu commented Jan 26, 2023 •
edited
Loading

tektoncd/community#931 is not merged yet, but in it, I added the guidance "Tests and documentation (where applicable) should always be part of the same pull request when introducing a change". To expand on this a bit in this context: If you remove tests first, we now have untested functionality (different values of the feature flag) in our code base. We wouldn't allow someone to introduce functionality without tests, so we should also not allow tests to be removed before the functionality is. The same is true for documentation. In terms of "easy rollback": if there was an issue with removing the feature flag and we wanted to revert the commit removing the flag, we would also want to revert removal of the tests. Therefore, I'd put this all in the same PR.

Changing our prow cluster configuration is a separate repo, so that can happen completely independently.

Makes sense! Thanks Lee, I will stick to the same PR then.

@JeromeJu
Copy link
Member

JeromeJu commented Jan 31, 2023 •
edited
Loading

Opened for cleanup after the removal of embedded-status feature flag in v0.45:

This was referenced Jan 31, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@JeromeJu JeromeJu

Labels
area/api Indicates an issue or PR that deals with the API. area/epic Issues that should be considered as Epics (aka multiple sub-tasks, …) kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
Tekton Community Roadmap
Status: Done
Milestone
Pipelines v0.45
Development

Successfully merging a pull request may close this issue.

[TEP100] Remove Taskruns and Runs Fields for PipelineRunStatus JeromeJu/pipeline
6 participants
@pritidesai @afrittoli @jerop @lbernick @JeromeJu @tekton-robot