-
Notifications
You must be signed in to change notification settings - Fork 267
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
Refactor TaskRun / PipelineRun component trees to separate definitions from runtime data #1814
Refactor TaskRun / PipelineRun component trees to separate definitions from runtime data #1814
Conversation
6f62755
to
657455c
Compare
/retest |
860bd75
to
dad3ecb
Compare
dad3ecb
to
6e7bb54
Compare
…s from runtime data Update the data processing done by the TaskRun/PipelineRun containers and their component trees to separate the Task definitions from the TaskRun runtime data instead of using our many custom representations that combined the two in different ways depending on need. This should help to resolve confusion around the origin of a number of fields used in the code, make it easier to maintain in future / easier for new contributors to understand, and also allows for additional functionality that was cumbersome or essentially impossible to achieve before. Since we're no longer bound by the definitions to construct the bulk of the UI, we can now more easily display generated steps for PipelineResources etc. giving a more complete representation of the run. This change has a dependency on Pipelines 0.17 for the correct step order, but otherwise should continue to work with earlier versions.
6e7bb54
to
c83f814
Compare
Had a call + screen share with Steve earlier to walk through the major changes and discuss additional scenarios to test. So far I'm not seeing any new issues. 🤞 |
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.
Code wise this looks good to me but there are a couple of more things to throw at this,
https://github.com/tektoncd/pipeline/blob/master/pkg/reconciler/pipelinerun/pipelinerun.go#L64
Would be worth double checking we handle these states gracefully ?
Checking:
Update: All but 2 of these error cases are handled the same as before this change, displaying the RunHeader with details of the error message. I couldn't find a way to trigger
I also couldn't reproduce |
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.
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: steveodonovan 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 |
Changes
#885
Update the data processing done by the TaskRun/PipelineRun containers and their component
trees to separate the Task definitions from the TaskRun runtime data instead of using
our many custom representations that combined the two in different ways depending on need.
This should help to resolve confusion around the origin of a number of fields used in the
code, make it easier to maintain in future / easier for new contributors to understand,
and also allows for additional functionality that was cumbersome or essentially impossible
to achieve before.
Since we're no longer bound by the definitions to construct the bulk of the UI, we can
now more easily display generated steps for PipelineResources etc. giving a more complete
representation of the run.
The following issues should benefit from this change, making them easier to address:
Also resolves #572
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide
for more details.