Adding support for taskSpec with pipelineSpec #1554

pritidesai · 2019-11-11T22:39:40Z

Changes

Now, this is possible:

apiVersion: tekton.dev/v1alpha1
kind: PipelineRun
metadata:
  name: pipelinerun-echo-greetings
spec:
  pipelineSpec:
    tasks:
      - name: echo-good-morning
        taskSpec:

Closes #872

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)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

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

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

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 embedding task specification with taskSpec under pipelineSpec such as:

apiVersion: tekton.dev/v1alpha1
kind: PipelineRun
metadata:
  name: pipelinerun-echo-greetings
spec:
  pipelineSpec:
    tasks:
      - name: echo-good-morning
        taskSpec:
        ...

tekton-robot · 2019-11-11T22:41:45Z

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/pipeline_validation.go	97.9%	98.1%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	83.9%	84.2%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.6%	94.9%	0.2
test/builder/pipeline.go	84.2%	84.6%	0.4

pritidesai · 2019-11-12T01:20:21Z

/assign @vdemeester

pritidesai · 2019-11-12T01:21:10Z

@vdemeester please let me know if there are any changes needed ...

dibyom

Thanks @pritidesai ...A few small nits. Otherwise looks good!
Also would you mind updating the Release Notes section of the PR description?

pkg/apis/pipeline/v1alpha1/pipeline_validation.go

Gopkg.lock

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

tekton-robot · 2019-11-13T19:36:35Z

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/pipeline_validation.go	97.9%	98.1%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	83.9%	84.2%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.6%	94.7%	0.1
test/builder/pipeline.go	84.2%	84.6%	0.4

pritidesai · 2019-11-13T19:43:23Z

/retest

pritidesai · 2019-11-13T19:54:47Z

Thanks @pritidesai ...A few small nits. Otherwise looks good!
Also would you mind updating the Release Notes section of the PR description?

Added same example to be clear and adding more value in Release Notes, would that be ok?

tekton-robot · 2019-11-13T20:16:08Z

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/pipeline_validation.go	97.9%	98.1%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	83.9%	84.2%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.6%	94.7%	0.1
test/builder/pipeline.go	84.2%	84.6%	0.4

pritidesai · 2019-11-13T20:57:41Z

/test pull-tekton-pipeline-build-tests

pritidesai · 2019-11-14T16:12:24Z

/retest

tekton-robot · 2019-11-15T19:53:13Z

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/pipeline_validation.go	97.9%	98.1%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	83.9%	84.2%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.6%	94.7%	0.1
test/builder/pipeline.go	84.2%	84.6%	0.4

tekton-robot · 2019-11-15T20:02:47Z

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/pipeline_validation.go	97.9%	98.1%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	83.9%	84.2%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.6%	94.7%	0.1
test/builder/pipeline.go	84.2%	84.6%	0.4

tekton-robot · 2019-11-15T20:10:27Z

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/pipeline_validation.go	97.9%	98.1%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	83.9%	84.2%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.6%	94.7%	0.1
test/builder/pipeline.go	84.2%	84.6%	0.4

pritidesai · 2019-11-15T20:49:09Z

Thanks @dibyom for the review, I have addressed all of your feedback/comments, its building as well (after multiple tries :( ) ...

vdemeester

/lgtm
/cc @bobcatfish

afrittoli

Thank you for this, very useful.
I have a couple of minor comments / questions - let me know what you think.

afrittoli · 2019-12-06T12:55:39Z

examples/pipelineruns/pipelinerun-with-pipelinespec-and-taskspec.yaml

+              command:
+                - echo
+              args:
+                - "$(inputs.params.MESSAGE)"


Would using $(params.GREETINGS) work here?

I find that one of the advantages of embedding is a more concise YAML. Since the task definition is embedded, it's not reusable (by definition), so there is little value in having input parameters in the task, if we can consume directly the pipeline parameters (and resources).

hey @afrittoli $(params.GREETINGS) will not work here as task is aware of only one input param named MESSAGE, here in this example, GREETINGS's value is assigned to task's input param MESSAGE. If we replace line 26 with $(params.GREETINGS), pod will have a output of:

$(params.GREETINGS)

I wonder if it should work or not though… My gut feeling as a user would be that I can use pipeline params in the embedded TaskSpec (which is not the case with this PR), right ?

I guess if we wanted that we'd need syntax like pipeline.params.FOO, right? Since both the pipeline and the task could have a param called FOO and they might have different intended meanings. So $(params.FOO) would be a bit confusing (to Tekton controller, but also to users). Or we could assume that task params always "win" if there's a same named param in both pipeline and task.

I guess if we wanted that we'd need syntax like pipeline.params.FOO, right? Since both the pipeline and the task could have a param called FOO and they might have different intended meanings. So $(params.FOO) would be a bit confusing (to Tekton controller, but also to users). Or we could assume that task params always "win" if there's a same named param in both pipeline and task.

Not entirely sure to be honest. I think the main question is, should the embedded TaskSpec in a PipelineSpec have the exact same feature fields as a normal TaskSpec ? Should we have Params at all in the embedded TaskSpec at all ? (from a user perspective, we don't really need to care about the implementation).

As a user, if I use an embedded TaskSpec, I kinda expect that the PipelineSpec.params can be used "as is" in the rest of the struct, which is not the case with the current state. I'm wondering if this is what others would expect or not 👼

oh i see. Yeah I like that. Pipeline has params but embedded task doesnt need them and just defers to the pipeline.

Implementation of that could be quite simple - embedded task spec is "hydrated" with a copy of the params from the pipeline before the taskrun is started.

added couple of more variations just for users to understand it until we change the existing behavior.

pkg/apis/pipeline/v1alpha1/pipeline_validation.go

pritidesai · 2019-12-06T22:30:00Z

Thank you for this, very useful.
I have a couple of minor comments / questions - let me know what you think.

Thanks @afrittoli, I have addressed your comments and questions. Let me know what you think.

afrittoli · 2019-12-10T09:48:36Z

NIT: In case you have to respin respin this for the test failures, it would be nice to document that the embedded task cannot refer do the PipelineRun parameters directly. we can do that as follow up too.
Thanks!
/lgtm

pritidesai · 2019-12-10T16:55:20Z

NIT: In case you have to respin respin this for the test failures, it would be nice to document that the embedded task cannot refer do the PipelineRun parameters directly. we can do that as follow up too.
Thanks!
/lgtm

thanks @afrittoli hopefully have fixed the build failure, had not pulled latest changes from master 😢

tekton-robot · 2019-12-10T16:56:09Z

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
test/builder/pipeline.go	87.3%	87.7%	0.4

pritidesai · 2019-12-10T17:40:52Z

/retest

pritidesai · 2019-12-10T18:09:28Z

Its ready for review again 🤔

tekton-robot · 2019-12-13T18:41:57Z

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
test/builder/pipeline.go	87.3%	87.7%	0.4

afrittoli · 2020-01-13T14:57:57Z

Thank you for the updates! Would you mind filing an issue about supporting access to pipeline params directly in the task spec, so we don't forget about it? I can also create it if you prefer so.
/lgtm

pritidesai · 2020-01-15T19:51:53Z

Hey @afrittoli just opened an issue here #1879, please correct it if my interpretation/understanding is off, also sorry for delay, not feeling well past few days, my twin boys passed something to me that they have been sick with 😢

pritidesai · 2020-01-15T21:06:27Z

/test pull-tekton-pipeline-build-tests

Now, this is possible: apiVersion: tekton.dev/v1alpha1 kind: PipelineRun metadata: name: pipelinerun-echo-greetings spec: pipelineSpec: tasks: - name: echo-good-morning taskSpec:

tekton-robot · 2020-01-15T22:03:49Z

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
test/builder/pipeline.go	87.9%	88.2%	0.3

pritidesai · 2020-01-16T00:10:19Z

@afrittoli its ready for review again, had to resolve conflicts 😭

vdemeester

Looks good to me 👍
(sorry for the conflicts, I may have a role in it 😛)
/meow

tekton-robot · 2020-01-16T08:48:20Z

@vdemeester:

In response to this:

Looks good to me 👍
(sorry for the conflicts, I may have a role in it 😛)
/meow

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 · 2020-01-16T08:48:23Z

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

~~OWNERS~~ [vdemeester]

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

afrittoli · 2020-01-17T09:56:30Z

Thank you for this!
/lgtm

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Nov 11, 2019

tekton-robot requested review from abayer and bobcatfish November 11, 2019 22:39

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 11, 2019

pritidesai mentioned this pull request Nov 11, 2019

Adding support for taskSpec with pipelineSpec - WIP #1546

Closed

3 tasks

tekton-robot assigned vdemeester Nov 12, 2019

dibyom reviewed Nov 13, 2019

View reviewed changes

pritidesai force-pushed the taskSpec-2 branch from 31436ef to 7b16783 Compare November 13, 2019 19:32

pritidesai force-pushed the taskSpec-2 branch from 7b16783 to 806a7b3 Compare November 13, 2019 20:11

pritidesai force-pushed the taskSpec-2 branch from 806a7b3 to 29aa113 Compare November 15, 2019 19:50

pritidesai force-pushed the taskSpec-2 branch from 29aa113 to fffb582 Compare November 15, 2019 20:00

pritidesai force-pushed the taskSpec-2 branch from fffb582 to e12ba8d Compare November 15, 2019 20:07

vdemeester reviewed Dec 3, 2019

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2019

afrittoli reviewed Dec 6, 2019

View reviewed changes

pritidesai force-pushed the taskSpec-2 branch from e12ba8d to 4f39949 Compare December 10, 2019 01:07

tekton-robot assigned afrittoli Dec 10, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 10, 2019

pritidesai force-pushed the taskSpec-2 branch from 4f39949 to f2628a7 Compare December 10, 2019 16:51

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 10, 2019

pritidesai force-pushed the taskSpec-2 branch from f2628a7 to 22adf79 Compare December 13, 2019 18:39

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2020

pritidesai mentioned this pull request Jan 15, 2020

Directly access Pipeline Params from taskSpec #1879

Closed

pritidesai force-pushed the taskSpec-2 branch from 22adf79 to d1f16f6 Compare January 15, 2020 20:39

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2020

Adding support for taskSpec with pipelineSpec

cd82e3c

Now, this is possible: apiVersion: tekton.dev/v1alpha1 kind: PipelineRun metadata: name: pipelinerun-echo-greetings spec: pipelineSpec: tasks: - name: echo-good-morning taskSpec:

pritidesai force-pushed the taskSpec-2 branch from d1f16f6 to cd82e3c Compare January 15, 2020 21:59

vdemeester approved these changes Jan 16, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 16, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2020

tekton-robot merged commit 5ca7763 into tektoncd:master Jan 17, 2020

ckadner mentioned this pull request Jun 5, 2020

Add metadata to PipelineTask to specific additional labels and annotations #2767

Closed

Adding support for taskSpec with pipelineSpec #1554

Adding support for taskSpec with pipelineSpec #1554

Conversation

pritidesai commented Nov 11, 2019 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Nov 11, 2019

pritidesai commented Nov 12, 2019

pritidesai commented Nov 12, 2019

dibyom left a comment • edited Loading

Choose a reason for hiding this comment

tekton-robot commented Nov 13, 2019

pritidesai commented Nov 13, 2019

pritidesai commented Nov 13, 2019

tekton-robot commented Nov 13, 2019

pritidesai commented Nov 13, 2019

pritidesai commented Nov 14, 2019

tekton-robot commented Nov 15, 2019

tekton-robot commented Nov 15, 2019

tekton-robot commented Nov 15, 2019

pritidesai commented Nov 15, 2019

vdemeester left a comment

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli Dec 6, 2019

Choose a reason for hiding this comment

pritidesai Dec 6, 2019

Choose a reason for hiding this comment

vdemeester Dec 9, 2019

Choose a reason for hiding this comment

ghost Dec 9, 2019

Choose a reason for hiding this comment

vdemeester Dec 9, 2019

Choose a reason for hiding this comment

ghost Dec 9, 2019

Choose a reason for hiding this comment

pritidesai Dec 10, 2019

Choose a reason for hiding this comment

pritidesai commented Dec 6, 2019

afrittoli commented Dec 10, 2019

pritidesai commented Dec 10, 2019

tekton-robot commented Dec 10, 2019

pritidesai commented Dec 10, 2019

pritidesai commented Dec 10, 2019

tekton-robot commented Dec 13, 2019

afrittoli commented Jan 13, 2020

pritidesai commented Jan 15, 2020 • edited Loading

pritidesai commented Jan 15, 2020

tekton-robot commented Jan 15, 2020

pritidesai commented Jan 16, 2020

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jan 16, 2020

tekton-robot commented Jan 16, 2020

afrittoli commented Jan 17, 2020

pritidesai commented Nov 11, 2019 •

edited

Loading

dibyom left a comment •

edited

Loading

pritidesai commented Jan 15, 2020 •

edited

Loading