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

Refactor TaskRun / PipelineRun component trees to separate definitions from runtime data #1814

Merged
merged 1 commit into from
Nov 10, 2020

Conversation

AlanGreene
Copy link
Member

@AlanGreene AlanGreene commented Nov 6, 2020

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.

@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 6, 2020
@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 6, 2020
@AlanGreene
Copy link
Member Author

/retest

…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.
@AlanGreene AlanGreene marked this pull request as ready for review November 9, 2020 15:50
@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 Nov 9, 2020
@AlanGreene
Copy link
Member Author

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. 🤞

Copy link
Member

@steveodonovan steveodonovan left a 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 ?

@AlanGreene
Copy link
Member Author

AlanGreene commented Nov 9, 2020

Checking:

  • CouldntGetCondition
  • CouldntGetPipeline
  • CouldntGetResource
  • CouldntGetTask
  • InvalidPipelineResourceBindings
  • InvalidServiceAccountMappings
  • InvalidTaskResultReference
  • InvalidWorkspaceBindings
  • ParameterMissing
  • ParameterTypeMismatch
  • PipelineInvalidGraph
  • PipelineRunCancelled
  • PipelineRunCouldntCancel
  • PipelineValidationFailed

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 PipelineInvalidGraph. By introducing a cycle as described in the code comment it's actually rejected by the admission webhook:

Error from server (BadRequest): error when creating "test.yaml": admission webhook "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: invalid value: couldn't add link between a-task and another-task: couldn't create link from another-task to a-task: cycle detected: a-task -> another-task -> a-task: spec.tasks

I also couldn't reproduce InvalidTaskResultReference, instead getting PipelineValidationFailed if I try to reference a result that doesn't exist, or no error if declaring a result that's not produced by the task.

Copy link
Member

@steveodonovan steveodonovan left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 10, 2020
@tekton-robot
Copy link
Contributor

[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 /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 Nov 10, 2020
@tekton-robot tekton-robot merged commit dd0e2a1 into tektoncd:master Nov 10, 2020
@AlanGreene AlanGreene deleted the taskrunsteps_refactor branch November 10, 2020 12:05
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. lgtm Indicates that a PR is ready to be merged. 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.

PipelineRun init errors should be displayed in the Tekton Dashboard
3 participants