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

RFC: "Script Mode" for task steps #781

Closed
imjasonh opened this issue Apr 23, 2019 · 7 comments · Fixed by #1432
Closed

RFC: "Script Mode" for task steps #781

imjasonh opened this issue Apr 23, 2019 · 7 comments · Fixed by #1432
Labels
design This task is about creating and discussing a design kind/feature Categorizes issue or PR as related to a new feature.

Comments

@imjasonh
Copy link
Member

(Much of this proposal is cribbed from proposals @ahmetb has previously made in other venues, ideas are not purely my own)

Expected Behavior

Users have an easy cruft-free way to express a multi-statement script in the body of a Task step, without having to understand advanced topics about containers, like what an entrypoint or a command is.

Users should only need to have passing familiarity with a shell environment to be successful using Tekton.

Actual Behavior

A user who wants to use shell-isms (|, >, $(...), ${VAR:-default}) has to understand how to set the step's command to a shell, pass -c as an arg, and even then has to stay on YAML's good side to stay successful:

steps:
- image: ubuntu
  command: bash
  args:
  - -c
  - |
    echo $(cat ${FILE:/tmp/file}) >> out.txt)
    cat out.txt

(this example is contrived, sure, but it's complicated by all the YAML garbage around it)

(It also fails to set -e or add && so that cat out.txt isn't run if the previous line failed. If you caught that bug, good for you! 🏅 )

Proposed Solution

In addition to the standard command, etc., steps could let users define these script strings directly in the API, to be invoked by a default shell. Compare:

steps:
- name: ubuntu
  script:
  - echo $(cat ${FILE:/tmp/file}) >> out.txt
  - cat out.txt

I hear what you're saying. But @imjasonh, the Kubernetes Container type doesn't include a field called script. You are correct. Which is why I propose changing our API from

type TaskSpec struct {
  Steps []corev1.Container `json:"steps"`
  ... 
}

to:

type Step struct {
  corev1.Container
  Script []string `json:"script"`
}

type TaskSpec struct {
  Steps []Step `json:"steps"`
}

This doesn't change how YAML is structured, only how objects are described in Go code. It'll be a pretty annoying mechanical change to update internal code and existing API callers, but I happen to think it's worth it.

Validation

If a step specifies a script value it cannot also specify args or command.

Implementation

When a step specifies script, the controller will translate that to a Container passed to a Pod more or less the same as it does today.

A step specified as:

steps:
- name: golang:1.11
  script:
  - go get ./...
  - cd cmd/app
  - go build -o myapp .
  - tar czvf app.tar.gz myapp

will be equivalent to a step specified as:

steps:
- name: golang:1.11
  command: /bin/bash
  args:
  - -c
  - |
    set -euo pipefail
    eval "$(set -x; go get ./...)"
    eval "$(set -x; cd cmd/app)"
    eval "$(set -x; go build -o .)"
    eval "$(set -x; tar czvf app.tar.gz ./app)"

Lingering problems/questions

This syntactic sugar does not solve some other notable usability issues, such as cd only doing anything within the context of a single step -- subsequent steps are back to /workspace until they cd somewhere else or set workingdir.

How this syntax works with input subtitutions, e.g., ${inputs.params.flags} might also lead to confusion among users, which we should design for.

Furthermore, what if the container image doesn't include a shell, or includes some exotic non-Bash shell we don't want to support? Should we allow users to override /bin/bash in that case? Can/should we detect it?

@abayer
Copy link
Contributor

abayer commented Apr 23, 2019

This seems worthwhile - I’ve been thinking along similar lines for Jenkins X syntax on top of Tekton’s. I’ll think on this more tomorrow, but in re the shell - we probably would want to default to /bin/sh and allow overriding it.

@vdemeester vdemeester added design This task is about creating and discussing a design kind/feature Categorizes issue or PR as related to a new feature. labels Apr 23, 2019
@dlorenc
Copy link
Contributor

dlorenc commented Apr 23, 2019

I really like the idea of decoupling a task definition from a container, we'll likely have to do this anyway for other types of Tasks. We've discussed things like JenkinsTasks, SpinnakerTasks, etc.

@imjasonh
Copy link
Member Author

@dlorenc This proposal still weds us to containers as an underlying implementation, but does start moving us away (optionally) in the API, and could open the door to future divergences from the container type, all inside the standard vanilla Task type.

I had not imagined this being a new Task sub-type, e.g., ScriptTask, without any container-isms involved. It still needs container-isms in the implementation (for now?). This would just extend the existing Task API, and make it not a 1:1 equivalent with containers.

@bobcatfish bobcatfish added the maybe-next-milestone For consideration when planning the next milestone label Jun 27, 2019
@bobcatfish
Copy link
Collaborator

@imjasonh do you want to try to get this in for 0.7? I'm gonna add it to the next milestone but lemme know if that's too soon.

@bobcatfish bobcatfish added this to the Pipelines 0.7 🐱 milestone Aug 12, 2019
@imjasonh
Copy link
Member Author

I don't have any specific urgency for it, but it should be fairly easy to implement if enough people think it's worth it and useful enough.

I'll add it to the WG agenda this week to get feedback before committing to it :)

(Thanks for pinging this to remind me!)

@skaegi
Copy link
Contributor

skaegi commented Sep 5, 2019

@imjasonh we're really interested in this and thinks it's important and would be happy to help if you would like it ;) (also... we have some urgency)

We've had similar functionality in IBM Cloud pipelines for a long time with one significant difference -- instead of an array of strings we use free-form YAML...
so...

script: |
  go get ./...
  cd cmd/app
  go build -o myapp .
  tar czvf app.tar.gz myapp

We use this to generate a _script file that we chmod +x ... and execute it.

What's kind of nice is you can also use hashbang syntax to control the interpreter so for example...

script: |
  #/bin/node
  console.log("Hello Tekton")

... is interpreted by Node. With that said shebang can be tricky and in some cases more precise control of the command is desirable.


With hindsight we might consider this...

If script is present ...

  • use the contents of script to create an "executable" _script (or similarly unique name) file which is placed in the workspace folder
  • if command is not present ./_script is executed (supports default shell and shebang case)
  • if args is not present ./_script is passed as an argument (supports more common alternate shell, node, python cases)
  • otherwise execute normally but still create the file (a little weird but the alternative would be to altogether ignore script which is inconsistent)

@imjasonh
Copy link
Member Author

imjasonh commented Sep 5, 2019

Nice, I like the idea of making it an executable file instead of passing things to some explicit shell.

I think it's simplest to enforce that script is mutually exclusive with command or args, rather than having to document how they'd all work together. I can't imagine a scenario where it would be easiest to express some script using both script and args, for example (if you have one, please let me know!).

I don't feel strongly about []string vs string with newlines, a nice thing about YAML is that it accepts a single string as a []string type and just interprets it the same as ["that-string"]. But that also means it (AIUI) would be backward-compatible to change a string-typed script field to a []string-typed field in a future release, if we decide we want to, because YAML wouldn't have to change.

I have a POC in https://github.com/ImJasonH/pipeline/tree/script (which is pretty far behind master now...) but it would need quite a bit of work to change to the _script model.

If you or someone on your team is interested in picking this up I'd be more than happy to help them, review designs/code, write tests, write docs, whatever.

@bobcatfish bobcatfish removed the maybe-next-milestone For consideration when planning the next milestone label Sep 6, 2019
@bobcatfish bobcatfish removed this from the Pipelines 0.7 🐱 milestone Sep 10, 2019
pradeepitm12 pushed a commit to openshift/tektoncd-pipeline that referenced this issue Jan 27, 2021
Port of tektoncd#3342:

The distroless nonroot image define a user with the uid 65532 and not 1001. The
deployment should use that uid to make sure it works anywhere.

Fixes tektoncd#781

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design This task is about creating and discussing a design kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
6 participants