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

WIP: Initial implementation of step scripts #1344

Closed
wants to merge 1 commit into from

Conversation

imjasonh
Copy link
Member

@imjasonh imjasonh commented Sep 23, 2019

This adds a script field to the Step type which, when specified,
results in a temporary generated executable script file being generated
and invoked containing the specified script items.

The result is an easier-to-use scripting option for users who just want
to invoke simple scripts. Generated scripts should be an implpementation
detail and will not persist between steps.

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.

Fixes #781

Release Notes

Support "script mode" for steps, which allows users to specify simple scripts inline in their YAML definition.

This adds a `script` field to the `Step` type which, when specified,
results in a temporary generated executable script file being generated
and invoked containing the specified script items.

The result is an easier-to-use scripting option for users who just want
to invoke simple scripts. Generated scripts should be an implpementation
detail and will not persist between steps.
@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 23, 2019
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ImJasonH

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

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Sep 23, 2019
@tekton-robot tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 23, 2019
@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/task_validation.go 77.1% 78.1% 1.0
pkg/reconciler/taskrun/resources/pod.go 89.7% 90.3% 0.5

- name: FOO
value: foooooooo
script:
- '#!/bin/bash'
Copy link
Member Author

@imjasonh imjasonh Sep 23, 2019

Choose a reason for hiding this comment

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

This makes me wonder whether Script shouldn't just be a string, which can contain newlines.

That would change these to:

steps:
- image: ubuntu
  env:
  - name: FOO
    value: foooooooo
  script: |
    #!/bin/bash
    set -euxo pipefail
    echo "Hello from Bash!"
    echo FOO is ${FOO}
    for i in {1..10}; do
      echo line $i
    done

- image: node
  script: |
    #!/usr/bin/env node
    console.log("Hello from Node!")

- image: python
  script: |
    #!/usr/bin/env python3
    print("Hello from Python!")

- image: perl
  script: |
    #!/usr/bin/perl
    print "Hello from Perl!"

...which I think is a bit clearer. We'd only lose the ability to have YAML comments inline, but that's probably not worth it.

@imjasonh
Copy link
Member Author

Closing in favor of #1432

@imjasonh imjasonh closed this Oct 17, 2019
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. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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.

RFC: "Script Mode" for task steps
3 participants