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

Add support for PipelineRun minimal embedded status #1617

Merged
merged 1 commit into from
Jul 1, 2022
Merged

Add support for PipelineRun minimal embedded status #1617

merged 1 commit into from
Jul 1, 2022

Conversation

abayer
Copy link
Contributor

@abayer abayer commented Jun 23, 2022 •
edited
Loading

Changes

fixes #1618

This is a dependency for tektoncd/pipeline#4954

Starting in Pipeline v0.35.0, we support a new approach to embedding TaskRun and Run statuses in PipelineRun.Status, as described in TEP-0100. When the embedded-status feature flag is set to minimal, the PipelineRun.Status.TaskRuns and PipelineRun.Status.Runs fields aren't populated. Instead, a new field, PipelineRun.Status.ChildReferences is populated with a list of the API version, kind, name, and PipelineTask name for each TaskRun or Run in the PipelineRun, requiring anything trying to see the status of the children of a PipelineRun to fetch those statuses directly.

Until Pipeline v0.44.0, the default value for the embedded-status feature flag is full, continuing the previous full embedded status behavior, but in v0.44.0, the default will be minimal, and in v0.45.0, the feature flag will go away, and the minimal embedded status will always be used. Therefore, the CLI needs to add support for minimal embedded status ASAP.

This change uses a helper function added in Pipeline v0.36.0 (which is just copied into the CLI code for now due to Chains not being compatible with Pipeline v0.36.0) to take a PipelineRun, check if it already has the deprecated TaskRuns and Runs maps populated (which will be the case for Pipeline pre-v0.35.0, and later Pipeline versions if the embedded-status feature flag is either full or both), returning those maps if they are populated, and otherwise generating those maps from the PipelineRun's ChildReferences and returning the resulting maps. The functions used in the CLI to fetch PipelineRuns will now call that helper function and set PipelineRun.Status.TaskRuns and PipelineRun.Status.Runs to the output of that function, ensuring that no other code changes should be needed in the CLI until those fields are removed.

/kind feature
/kind tep

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests (if functionality changed/added)
  • Run the code checkers with make check
  • Regenerate the manpages, docs and go formatting with make generated
  • Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

Add support for Pipeline's minimal embedded status feature flag.

@tekton-robot tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 23, 2022
@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 23, 2022
@abayer abayer force-pushed the pipeline-minimal-embedded-status branch 2 times, most recently from b14234f to ba09bb1 Compare June 23, 2022 18:50
@tekton-robot tekton-robot added kind/feature Categorizes issue or PR as related to a new feature. kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). labels Jun 23, 2022
@abayer abayer changed the title WIP: Add support for PipelineRun minimal embedded status Add support for PipelineRun minimal embedded status Jun 23, 2022
@tekton-robot 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 Jun 23, 2022
@abayer
Copy link
Contributor Author

abayer commented Jun 23, 2022

cc @vdemeester @afrittoli @jerop

@abayer
Copy link
Contributor Author

abayer commented Jun 23, 2022

/retest

@jerop
Copy link
Member

jerop commented Jun 23, 2022

@vinamra28 please take a look 🙏🏾

@tekton-robot
Copy link
Contributor

[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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 24, 2022
@abayer abayer force-pushed the pipeline-minimal-embedded-status branch from ba09bb1 to a00e153 Compare June 27, 2022 14:48
@abayer
Copy link
Contributor Author

abayer commented Jun 27, 2022

ping @vinamra28 @chmouel and anyone else from @tektoncd/cli-maintainers =)

@abayer
Copy link
Contributor Author

abayer commented Jun 27, 2022

/retest

@abayer abayer force-pushed the pipeline-minimal-embedded-status branch from a00e153 to 5b4237f Compare June 27, 2022 21:23
pradeepitm12
pradeepitm12 reviewed Jun 29, 2022
View reviewed changes
pkg/cmd/pipelinerun/logs_test.go Outdated
},
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

may be we can remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! I missed that - thanks for catching it!

pradeepitm12
pradeepitm12 reviewed Jun 29, 2022
View reviewed changes
pkg/pipelinerun/pipelinerun.go
return &pr, nil
}

// getFullPipelineTaskStatuses returns populated TaskRun and Run status maps for a PipelineRun from its ChildReferences.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@abayer abayer force-pushed the pipeline-minimal-embedded-status branch from 5b4237f to 082787a Compare June 29, 2022 14:08
@tekton-robot tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 29, 2022
@abayer abayer force-pushed the pipeline-minimal-embedded-status branch from 082787a to 7123767 Compare June 29, 2022 15:29
@abayer
Copy link
Contributor Author

abayer commented Jun 29, 2022

/retest

2 similar comments
@abayer
Copy link
Contributor Author

abayer commented Jun 29, 2022

/retest

@chmouel
Copy link
Member

chmouel commented Jun 30, 2022

/retest

@chmouel
Copy link
Member

chmouel commented Jun 30, 2022

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 30, 2022
afrittoli
afrittoli reviewed Jun 30, 2022
View reviewed changes
test/e2e-tests.sh
# Re-run the PipelineRun tests with "embedded-status" set to "minimal"
header "Running Go e2e tests with Pipeline embedded-status set to minimal"

set_minimal_embedded_status
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep testing the old behaviour too? Not sure how though, I guess that would mean an extra e2e job?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are still testing that - we're running the full set of tests with the default (full), and then updating the flag to minimal and rerunning the relevant tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided it wasn't worth adding an additional job given how few tests actually change behavior depending on the embedded-status value - it was markedly simpler and faster to get what we needed by just doing it this way. =)

@abayer abayer force-pushed the pipeline-minimal-embedded-status branch from 7123767 to 41cbf03 Compare July 1, 2022 11:28
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 1, 2022
@abayer
Copy link
Contributor Author

abayer commented Jul 1, 2022

/test pull-tekton-cli-unit-tests

@vinamra28
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 1, 2022
@tekton-robot tekton-robot merged commit bb539db into tektoncd:main Jul 1, 2022
@vinamra28 vinamra28 mentioned this pull request Jul 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afrittoli afrittoli afrittoli left review comments

@pradeepitm12 pradeepitm12 pradeepitm12 left review comments

@vdemeester vdemeester vdemeester approved these changes

@vinamra28 vinamra28 Awaiting requested review from vinamra28

Assignees

@chmouel chmouel

@vinamra28 vinamra28

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support PipelineRun minimal embedded status
8 participants
@abayer @jerop @tekton-robot @chmouel @vinamra28 @vdemeester @afrittoli @pradeepitm12