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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update some simple examples to use script mode #1469

Merged
merged 1 commit into from
Nov 27, 2019
Merged

Update some simple examples to use script mode #1469

merged 1 commit into from
Nov 27, 2019

Conversation

imjasonh
Copy link
Member

There are plenty more to do, but this was the lowest-hanging fruit. 馃崕

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.

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Oct 24, 2019
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 24, 2019
@ghost
Copy link

ghost commented Oct 24, 2019

script mode is nice.

/lgtm

@tekton-robot tekton-robot assigned ghost Oct 24, 2019
@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2019
vdemeester
vdemeester approved these changes Oct 25, 2019
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.

/lgtm
/meow

@imjasonh you may need to rebase to fix the build 馃

@tekton-robot
Copy link
Collaborator

@vdemeester: cat image

In response to this:

/lgtm
/meow

@imjasonh you may need to rebase to fix the build 馃

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 25, 2019
@bobcatfish
Copy link
Collaborator

Seems like you're getting hit for a linting error that couldn't possibly be your fault 馃

I1024 19:36:14.306] 2019/10/24 19:36:14 No errors found.
I1024 19:38:58.237] cmd/bash/main.go:66: G204: Subprocess launched with function call as argument or cmd arguments (gosec)
I1024 19:38:58.410] 	cmd := exec.Command("bash", "-c", *args)
I1024 19:38:58.410] pkg/client/clientset/versioned/typed/pipeline/v1alpha1/pipeline_client.go:100:45: setConfigDefaults - result 0 (error) is always nil (unparam)
I1024 19:38:58.411] func setConfigDefaults(config *rest.Config) error {
I1024 19:38:58.411]                                             ^

@bobcatfish
Copy link
Collaborator

/test pull-tekton-pipeline-integration-tests
/test pull-tekton-pipeline-build-tests

@vdemeester
Copy link
Member

/test pull-tekton-pipeline-integration-tests

4 similar comments
@vdemeester
Copy link
Member

/test pull-tekton-pipeline-integration-tests

@vdemeester
Copy link
Member

/test pull-tekton-pipeline-integration-tests

@vdemeester
Copy link
Member

/test pull-tekton-pipeline-integration-tests

@vdemeester
Copy link
Member

/test pull-tekton-pipeline-integration-tests

@afrittoli
Copy link
Member

@imjasonh it may be worth rebasing to get the fix to the cloud event test in.

@dibyom
Copy link
Member

dibyom commented Nov 13, 2019

/test pull-tekton-pipeline-integration-tests

1 similar comment
@vdemeester
Copy link
Member

/test pull-tekton-pipeline-integration-tests

afrittoli
afrittoli reviewed Nov 26, 2019
View reviewed changes
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

Nice work, thank you!
Missing one shebang, but that can also be updated later.
/lgtm

docs/tasks.md Show resolved Hide resolved
docs/tasks.md
@@ -597,14 +594,17 @@ Mounting multiple volumes:
spec:
steps:
- image: ubuntu
entrypoint: ["bash"]
args: ["-c", "curl https://foo.com > /var/my-volume"]
script: |
Copy link
Member

Choose a reason for hiding this comment

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

NIT: this might be even simpler with https://hub.docker.com/r/curlimages/curl

examples/taskruns/task-multiple-output-image.yaml Outdated
- -ce
- |
script: |
#!/usr/bin/env sh
set -ex
cat <<EOF > /builder/home/image-outputs/builtImage1/index.json
Copy link
Member

Choose a reason for hiding this comment

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

Not related to this patch, but I think we should change this to a variable.
I think $(outputs.resources.builtImage1.path)/index.json should work?

@afrittoli
Copy link
Member

/test pull-tekton-pipeline-integration-tests

@afrittoli
Copy link
Member

It would be nice to get this in v0.9.0

@vdemeester vdemeester added this to the Pipelines 0.9 馃惐 milestone Nov 26, 2019
@vdemeester
Copy link
Member

@afrittoli yeah definitely :)

@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2019
vdemeester
vdemeester approved these changes Nov 26, 2019
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.

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2019
@imjasonh
Copy link
Member Author

/test pull-tekton-pipeline-integration-tests

@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 26, 2019
vdemeester
vdemeester approved these changes Nov 27, 2019
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.

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 27, 2019
@tekton-robot
Copy link
Collaborator

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

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

/test pull-tekton-pipeline-build-tests

@tekton-robot tekton-robot merged commit aea187a into tektoncd:master Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afrittoli afrittoli afrittoli left review comments

@vdemeester vdemeester vdemeester approved these changes

@bobcatfish bobcatfish Awaiting requested review from bobcatfish

@dibyom dibyom Awaiting requested review from dibyom

Assignees

@vdemeester vdemeester

@afrittoli afrittoli

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
Pipelines 0.9 馃惐
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@imjasonh @tekton-robot @bobcatfish @vdemeester @afrittoli @dibyom @googlebot