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

Consolidate Results validation + add Results to Run status #292

Merged
merged 5 commits into from
Dec 4, 2018
Merged

Consolidate Results validation + add Results to Run status #292

merged 5 commits into from
Dec 4, 2018

Conversation

bobcatfish
Copy link
Collaborator

When a user kicks off a run, they will provide an endpoint to upload logs to (initial implementation will be in #107). The corresponding fields in status will indicate where the logs actually got uplaoded to.

Once we actually get to #107, and especially once we start supporting endpoints other than GCS, we may find this isn't useful and remove it.

As a bonus:

  • Remove name field from Results
    In the interests in keeping our spec as lean and simple as it can possibly be, at the moment there is no reason why our Results config needs to have a name, so let's remove it. This is the last unnecessary field we have in our yamls that I'm aware of, so this fixes Concise Yaml for pipeline declaration. #138
  • Consolidate logic for handling results config for PipelineRun + TaskRun
    It turned out TaskRun and PipelienRun were using slightly different logic for validating the results config, even tho it should be the same for both. Now they share the logic and in both cases the config is optional.
  • Simplify results config by removing log key
    Since currently the only kind of result is log, we can remove that key. We change change this if we decide to have other kinds of results later.

Fixes #146
Fixes #138

@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 1, 2018
@bobcatfish
Simplify results config by removing log key
8027191 
Since currently the only kind of result is `log`, we can remove that
key. We change change this if we decide to have other kinds of results
later.
@bobcatfish bobcatfish force-pushed the taskrun_result_status branch 2 times, most recently from a01e6f4 to 864024d Compare December 1, 2018 18:29
@bobcatfish bobcatfish force-pushed the taskrun_result_status branch from 864024d to 4f516ee Compare December 1, 2018 18:43
@bobcatfish
Consolidate logic for handling results config for PipelineRun + TaskRun
9953d22 
It turned out TaskRun and PipelienRun were using slightly different
logic for validating the results config, even tho it should be the same
for both. Now they share the logic and in both cases the config is
optional.
@bobcatfish
Remove name field from Results
ae73170 
In the interests in keeping our spec as lean and simple as it can
possibly be, at the moment there is no reason why our Results config
needs to have a name, so let's remove it.

This is the last unnecessary field we have in our yamls that I'm aware
of, so this fixes tektoncd#138
@bobcatfish
Add Results fields to TaskRun and PipelineRun status
a7b0e3a 
When a user kicks off a run, they will provide an endpoint to upload
logs to (initial implementation will be in #107). The corresponding
fields in `status` will indicate where the logs actually got uplaoded
to.

Once we actually get to #107, and especially once we start supporting
endpoints other than GCS, we may find this isn't useful and remove it.

Fixes tektoncd#146
@bobcatfish bobcatfish force-pushed the taskrun_result_status branch from 4f516ee to a7b0e3a Compare December 1, 2018 18:47
vdemeester
vdemeester approved these changes Dec 3, 2018
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

SGTM 🐯

@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, 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

@abayer
Copy link
Contributor

abayer commented Dec 3, 2018

Since currently the only kind of result is log, we can remove that key. We change change this if we decide to have other kinds of results later.

Yeah, that seems fine to me for now, but I know I'm going to want to eventually have more possible types.

@bobcatfish
Copy link
Collaborator Author

I know I'm going to want to eventually have more possible types.

Super cool @abayer ! If you feel like writing up an issue or doc with some details about what types you'd like to see that would be super cool! No worries if it's too premature. and p.s. @tejal29 would be a good contact to continue to discuss the results design

@bobcatfish
Make results in Run status optional
421dd8d 
If it's optional in the spec, it should probably be optional in the
Status, since the behaviour would probably be not to upload the logs
anywhere if no endpoint is provided.

Possibly in the future these could become mandator, depending how we
design the results store interface.
@bobcatfish
Copy link
Collaborator Author

Ready for another look @pivotal-nader-ziada !

@knative-metrics-robot
Copy link

The following is the coverage report on pkg/.
Say /test pull-knative-build-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go 88.0% 92.3% 4.3
pkg/apis/pipeline/v1alpha1/result_types.go Do not exist 84.2%
pkg/apis/pipeline/v1alpha1/taskrun_validation.go 94.6% 91.3% -3.3

@nader-ziada
Copy link
Member

/lgtm

/meow space :D

@knative-prow-robot
Copy link

@pivotal-nader-ziada: Bad category. Please see https://api.thecatapi.com/api/categories/list

In response to this:

/lgtm

/meow space :D

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2018
@knative-prow-robot knative-prow-robot merged commit b72bd1f into tektoncd:master Dec 4, 2018
@bobcatfish
Copy link
Collaborator Author

@pivotal-nader-ziada: Bad category. Please see https://api.thecatapi.com/api/categories/list

nooooo

/meow space

@knative-prow-robot
Copy link

@bobcatfish: cat image

In response to this:

@pivotal-nader-ziada: Bad category. Please see https://api.thecatapi.com/api/categories/list

nooooo

/meow space

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nader-ziada nader-ziada nader-ziada left review comments

@vdemeester vdemeester vdemeester approved these changes

@shashwathi shashwathi Awaiting requested review from shashwathi

Assignees
No one assigned
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TaskRun should have Results in Status Concise Yaml for pipeline declaration.
6 participants
@bobcatfish @knative-prow-robot @abayer @knative-metrics-robot @nader-ziada @vdemeester