-
Notifications
You must be signed in to change notification settings - Fork 249
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
pipeline start supports taskRunSpec argument #1969
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @haedaal. 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 Once the patch is verified, the new status will be reflected by the 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. |
@@ -0,0 +1,6 @@ | |||
- pipelineTaskName: unit-test-task |
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.
I referred #1088 writing this pr, and it's mostly the same but here hardcoded pipelineTaskName maybe seems fragile to future change. maybe ok?
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.
@haedaal what could be the other options ? multiple flags --tasrun-spec
, one fore each task ? what else ?
I think the one is ok, but I won't if there could be other alternative to look into.
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.
may be we can add a check in parsing to have multiple tasks
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.
@vdemeester I feel like there's no better alternatives, as it looks exactly same as k8s spec. I think simplicity is worth a bit of fragility.
test/e2e/pipeline/pipeline_test.go
Outdated
@@ -305,6 +305,23 @@ Waiting for logs to be available... | |||
} | |||
}) | |||
|
|||
t.Run("Start PipelineRun with --task-run-spec", func(t *testing.T) { |
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.
When I try to run e2e test, I'm seeing following log
package github.com/tektoncd/cli/test/e2e/pipeline: build constraints exclude all Go files in /Users/minjaekim/workspace/oss/cli/test/e2e/pipeline
So I've written this code, but never have run it. What should I do?
@@ -0,0 +1,6 @@ | |||
- pipelineTaskName: unit-test-task |
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.
@haedaal what could be the other options ? multiple flags --tasrun-spec
, one fore each task ? what else ?
I think the one is ok, but I won't if there could be other alternative to look into.
/ok-to-test |
@@ -0,0 +1,6 @@ | |||
- pipelineTaskName: unit-test-task |
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.
need license header here
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.
fixed @ 0663311
pkg/pods/task_run_spec.go
Outdated
@@ -0,0 +1,39 @@ | |||
// Copyright © 2020 The Tekton Authors. |
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.
this should be 2023
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.
fixed @ 5f4ffd5
pkg/pods/task_run_spec_test.go
Outdated
@@ -0,0 +1,98 @@ | |||
// Copyright © 2020 The Tekton Authors. |
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.
this should be 2023
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.
fixed @ 5f4ffd5
@@ -0,0 +1,20 @@ | |||
# Copyright 2020 The Tekton Authors. |
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.
this should be 2023
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.
fixed @ 5f4ffd5
@@ -0,0 +1,20 @@ | |||
# Copyright 2020 The Tekton Authors. |
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.
this should be 2023
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.
fixed @ 5f4ffd5
pkg/pods/testdata/taskrunspec.yaml
Outdated
@@ -0,0 +1,20 @@ | |||
# Copyright 2020 The Tekton Authors. |
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.
this should be 2023
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.
fixed @ 5f4ffd5
test/e2e/pipeline/pipeline_test.go
Outdated
tkn.MustSucceed(t, "pipeline", "start", tePipelineName, | ||
"-p=filename=output", | ||
"-w=name=shared-data,emptyDir=", | ||
"--taskrun-spec="+helper.GetResourcePath("/taskrunspec/taskrunspec.yaml"), |
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.
the path seems incorrect here
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.
fixed @ ede2079
/retest |
Issues go stale after 90d of inactivity. /lifecycle stale Send feedback to tektoncd/plumbing. |
Stale issues rot after 30d of inactivity. /lifecycle rotten Send feedback to tektoncd/plumbing. |
/remove-lifecycle rotten |
Would love to get this in, can you please squash the commit into one, with nice commit msg. |
/test all |
@haedaal: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Changes
Add taskRunSpec argument on
tkn pipeline start
closes #951
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
make check
make generated
See the contribution guide
for more details.
Release Notes