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

Allow script mode to accept scripts that do not start with a shebang. #1691

Merged
merged 1 commit into from
Dec 5, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions docs/tasks.md
Original file line number Diff line number Diff line change
Expand Up @@ -156,10 +156,11 @@ If this field is present, the step cannot specify `command`.
When specified, a `script` gets invoked as if it were the contents of a file in
the container. Any `args` are passed to the script file.

Scripts should start with a
[shebang](https://en.wikipedia.org/wiki/Shebang_(Unix)) line to declare what
tool should be used to interpret the script. That tool must then also be
available within the step's container.
Scripts that do not start with a shebang
[shebang](https://en.wikipedia.org/wiki/Shebang_(Unix)) line will use a default
value of `#!/bin/sh`, although users can override this by starting their script
Copy link

Choose a reason for hiding this comment

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

small suggestion, feel free to ignore since this is a dockerism, but #!/usr/bin/env sh will account for situations where sh is in /usr/bin or /usr/sbin of the image instead of /bin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I considered that but decided to stick with the docker version, I suppose there might be images with sh but not env?

Copy link

Choose a reason for hiding this comment

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

Yeah, totally fair point.

with a shebang to declare what tool should be used to interpret the script.
That tool must then also be available within the step's container.

This allows you to execute a Bash script, if the image includes `bash`:

Expand Down
3 changes: 3 additions & 0 deletions examples/taskruns/step-script.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ spec:
default: param-value

steps:
- name: noshebang
image: ubuntu
script: echo "no shebang"
- name: bash
image: ubuntu
env:
Expand Down
6 changes: 0 additions & 6 deletions pkg/apis/pipeline/v1alpha1/task_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,6 @@ func validateSteps(steps []Step) *apis.FieldError {
Paths: []string{"script"},
}
}
if !strings.HasPrefix(strings.TrimSpace(s.Script), "#!") {
return &apis.FieldError{
Message: "script must start with a shebang (#!)",
Paths: []string{"script"},
}
}
}

if s.Name == "" {
Expand Down
14 changes: 0 additions & 14 deletions pkg/apis/pipeline/v1alpha1/task_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -631,20 +631,6 @@ func TestTaskSpecValidateError(t *testing.T) {
Message: `multiple volumes with same name "workspace"`,
Paths: []string{"volumes.name"},
},
}, {
name: "step with script without shebang",
fields: fields{
Steps: []v1alpha1.Step{{
Container: corev1.Container{
Image: "my-image",
},
Script: "does not begin with shebang",
}},
},
expectedError: apis.FieldError{
Message: "script must start with a shebang (#!)",
Paths: []string{"steps.script"},
},
}, {
name: "step with script and command",
fields: fields{
Expand Down
3 changes: 2 additions & 1 deletion pkg/pod/pod_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,7 @@ func TestMakePod(t *testing.T) {
Name: "one",
Image: "image",
},
Script: "echo hello from step one",
Script: "#!/bin/sh\necho hello from step one",
}, {
Container: corev1.Container{
Name: "two",
Expand All @@ -462,6 +462,7 @@ print("Hello from Python")`,
Args: []string{"-c", `tmpfile="/tekton/scripts/script-0-mz4c7"
touch ${tmpfile} && chmod +x ${tmpfile}
cat > ${tmpfile} << 'script-heredoc-randomly-generated-mssqb'
#!/bin/sh
echo hello from step one
script-heredoc-randomly-generated-mssqb
tmpfile="/tekton/scripts/script-1-78c5n"
Expand Down
14 changes: 13 additions & 1 deletion pkg/pod/script.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package pod
import (
"fmt"
"path/filepath"
"strings"

"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/names"
Expand All @@ -28,6 +29,7 @@ import (
const (
scriptsVolumeName = "scripts"
scriptsDir = "/tekton/scripts"
defaultShebang = "#!/bin/sh\n"
)

var (
Expand Down Expand Up @@ -68,6 +70,16 @@ func convertScripts(shellImage string, steps []v1alpha1.Step) (*corev1.Container
continue
}

// Check for a shebang, and add a default if it's not set.
// The shebang must be the first non-empty line.
cleaned := strings.TrimSpace(s.Script)
hasShebang := strings.HasPrefix(cleaned, "#!")

script := s.Script
if !hasShebang {
script = defaultShebang + s.Script
}

// At least one step uses a script, so we should return a
// non-nil init container.
placeScripts = true
Expand All @@ -88,7 +100,7 @@ touch ${tmpfile} && chmod +x ${tmpfile}
cat > ${tmpfile} << '%s'
%s
%s
`, tmpFile, heredoc, s.Script, heredoc)
`, tmpFile, heredoc, script, heredoc)

// Set the command to execute the correct script in the mounted
// volume.
Expand Down
32 changes: 30 additions & 2 deletions pkg/pod/script_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,23 @@ func TestConvertScripts(t *testing.T) {
}}

gotInit, got := convertScripts(images.ShellImage, []v1alpha1.Step{{
Script: "script-1",
Script: `#!/bin/sh
script-1`,
Container: corev1.Container{Image: "step-1"},
}, {
// No script to convert here.
Container: corev1.Container{Image: "step-2"},
}, {
Script: "script-3",
Script: `
#!/bin/sh
script-3`,
Container: corev1.Container{
Image: "step-3",
VolumeMounts: preExistingVolumeMounts,
Args: []string{"my", "args"},
},
}, {
Script: `no-shebang`,
Container: corev1.Container{
Image: "step-3",
VolumeMounts: preExistingVolumeMounts,
Expand All @@ -77,13 +87,22 @@ func TestConvertScripts(t *testing.T) {
Args: []string{"-c", `tmpfile="/tekton/scripts/script-0-mz4c7"
touch ${tmpfile} && chmod +x ${tmpfile}
cat > ${tmpfile} << 'script-heredoc-randomly-generated-mssqb'
#!/bin/sh
script-1
script-heredoc-randomly-generated-mssqb
tmpfile="/tekton/scripts/script-2-78c5n"
touch ${tmpfile} && chmod +x ${tmpfile}
cat > ${tmpfile} << 'script-heredoc-randomly-generated-6nl7g'
#!/bin/sh
script-3
script-heredoc-randomly-generated-6nl7g
tmpfile="/tekton/scripts/script-3-j2tds"
touch ${tmpfile} && chmod +x ${tmpfile}
cat > ${tmpfile} << 'script-heredoc-randomly-generated-vr6ds'
#!/bin/sh
no-shebang
script-heredoc-randomly-generated-vr6ds
`},
VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount},
}
Expand All @@ -98,6 +117,15 @@ script-heredoc-randomly-generated-6nl7g
Command: []string{"/tekton/scripts/script-2-78c5n"},
Args: []string{"my", "args"},
VolumeMounts: append(preExistingVolumeMounts, scriptsVolumeMount),
}, {
Image: "step-3",
Command: []string{"/tekton/scripts/script-3-j2tds"},
Args: []string{"my", "args"},
VolumeMounts: []corev1.VolumeMount{
{Name: "pre-existing-volume-mount", MountPath: "/mount/path"},
{Name: "another-one", MountPath: "/another/one"},
{Name: "scripts", MountPath: "/tekton/scripts"},
},
}}
if d := cmp.Diff(wantInit, gotInit); d != "" {
t.Errorf("Init Container Diff (-want, +got): %s", d)
Expand Down