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

Fix for #2362, adding support per TaskRun runtime settings. #2389

Merged
merged 1 commit into from
May 11, 2020

Conversation

NikeNano
Copy link

@NikeNano NikeNano commented Apr 14, 2020

Changes

This PR updates the Pipelinerun object to allow for setting task specific podTemplates to handle variations on different task. This will allow users to not only relay on a podTemplate for all task but instead it can be set on task level giving it a higher degree of freedom. More information can be found here:

  • Updated the PipelineRunSpec to have a slice of PipelineRunTaskSpec which allows task specific settings.
  • Set the TaskRun values based upon the PipelineRunTaskSpec if available.
  • Added tests

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.

Double check this list of stuff that's easy to miss:

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Allow PodTemplate to be set on task level at run time. 

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Apr 14, 2020
@tekton-robot tekton-robot requested review from abayer and a user April 14, 2020 15:11
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 14, 2020
@tekton-robot
Copy link
Collaborator

Hi @NikeNano. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@tekton-robot tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 14, 2020
@NikeNano
Copy link
Author

Should I squash my commits? I see that I don't follow the expected standards.

@NikeNano
Copy link
Author

/assign @dibyom

@dibyom
Copy link
Member

dibyom commented Apr 15, 2020

/ok-to-test

@tekton-robot tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 15, 2020
Copy link
Member

@dibyom dibyom left a comment

Choose a reason for hiding this comment

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

Thanks @NikeNano....I added a few comments. A couple of things to add would be 1. some docs 2. a YAML example (in the examples folder) that utilizes this feature!
(Also, yes, before we merge we'd like to rebase the commits 👼 )

pkg/apis/pipeline/v1alpha1/pipelinerun_types.go Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1alpha1/pipelinerun_types.go Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/pipelinerun_types.go Outdated Show resolved Hide resolved
@NikeNano NikeNano marked this pull request as draft April 16, 2020 15:20
@tekton-robot tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 17, 2020
@NikeNano NikeNano force-pushed the run_time_settings branch 2 times, most recently from 0ffb3b3 to 25cf43a Compare April 17, 2020 09:48
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/pipelinerun_types.go 88.5% 90.9% 2.4
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 85.7% 88.1% 2.4

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/pipelinerun_types.go 88.5% 90.9% 2.4
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 85.7% 88.1% 2.4

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/pipelinerun_types.go 88.5% 90.9% 2.4
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 85.7% 88.1% 2.4

@NikeNano
Copy link
Author

Could some one help me out with interpreting the errors from the tests? I don't get the error messages.

@NikeNano NikeNano force-pushed the run_time_settings branch from d2a6cf5 to e9247fe Compare April 18, 2020 15:06
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/pipelinerun_types.go 88.5% 90.9% 2.4
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 85.7% 88.1% 2.4

@NikeNano NikeNano marked this pull request as ready for review April 21, 2020 07:46
@NikeNano
Copy link
Author

Hi @dibyom could you take a look again :) Thanks!

@dibyom
Copy link
Member

dibyom commented Apr 22, 2020

Build tests are failing due to:

I0418 15:07:36.162] =============================
I0418 15:07:36.163] ==== RUNNING BUILD TESTS ====
I0418 15:07:36.163] =============================
I0418 15:07:36.166] -------------------------------------------
I0418 15:07:36.167] ---- Checking go code style with gofmt ----
I0418 15:07:36.167] -------------------------------------------
I0418 15:07:38.692] diff -u pkg/apis/pipeline/v1alpha1/pipelinerun_types.go.orig pkg/apis/pipeline/v1alpha1/pipelinerun_types.go
I0418 15:07:38.692] --- pkg/apis/pipeline/v1alpha1/pipelinerun_types.go.orig	2020-04-18 15:07:37.883833184 +0000
I0418 15:07:38.693] +++ pkg/apis/pipeline/v1alpha1/pipelinerun_types.go	2020-04-18 15:07:37.883833184 +0000
I0418 15:07:38.693] @@ -220,6 +220,7 @@
I0418 15:07:38.693]  	}
I0418 15:07:38.694]  	return false
I0418 15:07:38.694]  }
I0418 15:07:38.694] +
I0418 15:07:38.695]  // PipelineTaskRunSpec  can be used to configure specific
I0418 15:07:38.695]  // specs for a concrete Task
I0418 15:07:38.695]  type PipelineTaskRunSpec struct {

@NikeNano
Copy link
Author

@dibyom is there anything more that I can improve?

Copy link
Member

@dibyom dibyom left a comment

Choose a reason for hiding this comment

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

Couple of minor comments...mostly around tests. And would you mind filling out the Release notes section of the PR description? Otherwise, LGTM!

docs/pipelineruns.md Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/pipelinerun_types.go Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1alpha1/pipelinerun_types_test.go Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1alpha1/pipelinerun_types_test.go Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1alpha1/pipelinerun_types_test.go Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1alpha1/pipelinerun_types_test.go Outdated Show resolved Hide resolved
@NikeNano
Copy link
Author

Couple of minor comments...mostly around tests. And would you mind filling out the Release notes section of the PR description? Otherwise, LGTM!

I will fix it :)

Currently it is not possible to set task run specs on each individual tasks. This PR aims to fix that and give the user more flexibility to set podTemplate for each task.

Co-Authored-By: Daniel Helfand <[email protected]>
@NikeNano NikeNano force-pushed the run_time_settings branch from ed0e1a0 to 5440c4e Compare May 4, 2020 08:22
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/pipelinerun_types.go 88.5% 90.9% 2.4
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 85.7% 88.1% 2.4
pkg/reconciler/pipelinerun/pipelinerun.go 72.4% 72.4% 0.1

@NikeNano
Copy link
Author

NikeNano commented May 4, 2020

/test pull-tekton-pipeline-integration-tests

@dibyom
Copy link
Member

dibyom commented May 4, 2020

Looks like one of the YAML tests timed out

/test pull-tekton-pipeline-integration-tests

@NikeNano
Copy link
Author

NikeNano commented May 4, 2020

@dibyom the issues seem to be solved, the test seem to be a bit flaky, often time out :(

@dibyom
Copy link
Member

dibyom commented May 4, 2020

Indeed, that test seems to be flaky.
/approve

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2020
@NikeNano
Copy link
Author

@dibyom is there something more to do in order for this to be merged?

@vdemeester vdemeester added area/api Indicates an issue or PR that deals with the API. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels May 11, 2020
@vdemeester
Copy link
Member

/cc @imjasonh @bobcatfish @afrittoli

@bobcatfish
Copy link
Collaborator

Let's do it! Haven't looked in much detail but from me re the API change at least:

/approve

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, dibyom

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

@vdemeester
Copy link
Member

/lgtm

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. area/api Indicates an issue or PR that deals with the API. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants