Add Script Support to Sidecars #1987

tomgeorge · 2020-01-30T20:27:00Z

Changes

This makes a sidecar behave like a Step, and allows the ability to pass a script argument.

This enables #1856.

To implement this, I created a Sidecar type that was the same as Step.

I then changed convertScripts to accept an image, a slice of steps, and a slice of sidecars, and return the init container, and two lists of containers: one for steps, and one for sidecars.

I also refactored convertScripts to call a helper function, convertListOfSteps, that takes a list of steps to convert, a pointer to the init container to add to, a pointer to placeScripts, and a format string for writing out the heredoc and temp file names. It returns the converted list of containers. convertListOfSteps does most of the work that convertScripts does. It might be a little ugly but it seemed better than putting everything in convertScripts. Unfortunately this doesn't translate very well to the Github PR viewer, but git diff works fine

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

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.

Release Notes

Add script support to sidecars

This makes a sidecar behave like a Step, and allows the ability to pass a script argument. This enables tektoncd#1856. Signed-off-by: Tom George <tg82490@gmail.com>

tekton-robot · 2020-01-30T20:27:15Z

Hi @tomgeorge. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

dibyom · 2020-01-30T23:14:08Z

/ok-to-test

chmouel · 2020-01-31T09:32:46Z

Nice! thank you! I think one thing missing would be a yaml in the examples directory which are used in CI for validation too..

tomgeorge · 2020-01-31T16:00:47Z

I'll add an example this evening

Signed-off-by: Tom George <tg82490@gmail.com>

vdemeester

/meow

tekton-robot · 2020-02-03T13:36:06Z

@vdemeester:

In response to this:

/meow

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 · 2020-02-03T13:36:11Z

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

~~OWNERS~~ [vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ghost · 2020-02-03T14:25:42Z

examples/taskruns/sidecar-ready-script.yaml

+    - name: check-ready
+      image: ubuntu
+      # The step will only succeed if the sidecar has written this file, which
+      # it does 5s after it starts, before it reports Ready.


I think I'm missing something here - where is the 5s delay happening? And what is meant here by the sidecar reporting Ready?

That was me copy/pasting another example without deleting the comments. It isn't really waiting for anything to be ready.

ghost · 2020-02-03T14:29:51Z

pkg/pod/pod_test.go

+			}}},
+			Sidecars: []v1alpha1.Sidecar{
+				{
+					Container: corev1.Container{


Suggest collapsing these brackets to match format with the Step on line 404. Similarly for the init container on line 422.

ghost · 2020-02-03T14:33:13Z

pkg/pod/script.go

+//
+// It iterates through the list of steps (or sidecars), generates the script file name and heredoc termination string,
+// adds an entry to the init container args, sets up the step container to run the script, and sets the volume mounts.
+func convertListOfSteps(steps []v1alpha1.Step, initContainer *corev1.Container, placeScripts *bool, format string) []corev1.Container {


suggest "namePrefix" instead of "format" here, since the value isn't expected to be a format string.

ghost · 2020-02-03T14:43:48Z

pkg/pod/script.go

 //
 // It does this by prepending a container that writes specified Script bodies
 // to executable files in a shared volumeMount, then produces Containers that
 // simply run those executable files.
-func convertScripts(shellImage string, steps []v1alpha1.Step) (*corev1.Container, []corev1.Container) {
+func convertScripts(shellImage string, steps []v1alpha1.Step, sidecars []v1alpha1.Sidecar) (*corev1.Container, []corev1.Container, []corev1.Container) {


Rather than amending the signature and adding the extra func to account for Sidecars, couldn't the caller (MakePod) simply execute this function twice? Once for Steps and once for Sidecars?

Yes you can definitely do the same thing that way. I think in order to do that you need to change the signature of convertScripts anyways to include context about the namePrefix. Would you prefer I changed it to that?

That would be excellent, thank you @tomgeorge !

Signed-off-by: Tom George <tg82490@gmail.com>

tomgeorge · 2020-02-04T03:06:13Z

@sbwsg I made the formatting/comment changes you requested. I need to think a little bit more about the best way to make the convertScript change, because if you call it twice in MakePod you have to keep track of the init container, instead of convertScripts just handing you one if you need it. Currently, convertScripts handles the init container and appends to it appropriately.

Signed-off-by: Tom George <tg82490@gmail.com>

tomgeorge · 2020-02-04T04:36:10Z

/test pull-tekton-pipeline-integration-tests

ghost · 2020-02-04T13:13:21Z

@tomgeorge building up the script in the init container definitely complicates things. Looking at it again I think your original solution is good. I was initially wary of the passing a pointer to the placeScripts bool and init container but given the way the code worked before this change I think yours is a sound approach. So I'm happy to leave things as-is with the two formatting changes.

/lgtm

Add Script Support to Sidecars

96c2cf7

This makes a sidecar behave like a Step, and allows the ability to pass a script argument. This enables tektoncd#1856. Signed-off-by: Tom George <tg82490@gmail.com>

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jan 30, 2020

tekton-robot requested review from dibyom and imjasonh January 30, 2020 20:27

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 30, 2020

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 30, 2020

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 30, 2020

Add an example taskrun showcasing a sidecar with a script field

2e0ef6f

Signed-off-by: Tom George <tg82490@gmail.com>

vdemeester approved these changes Feb 3, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2020

ghost reviewed Feb 3, 2020

View reviewed changes

Made formatting and comment changes in response to code review

ede66f7

Signed-off-by: Tom George <tg82490@gmail.com>

Change format parameter in convertListOfSteps to namePrefix

753e286

Signed-off-by: Tom George <tg82490@gmail.com>

tekton-robot assigned ghost Feb 4, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2020

tekton-robot merged commit f4e2e44 into tektoncd:master Feb 4, 2020

vdemeester mentioned this pull request Feb 18, 2020

Script support for sidecar #1856

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Script Support to Sidecars #1987

Add Script Support to Sidecars #1987

tomgeorge commented Jan 30, 2020 •

edited

Loading

tekton-robot commented Jan 30, 2020

dibyom commented Jan 30, 2020

chmouel commented Jan 31, 2020

tomgeorge commented Jan 31, 2020

vdemeester left a comment

tekton-robot commented Feb 3, 2020

tekton-robot commented Feb 3, 2020

ghost Feb 3, 2020

tomgeorge Feb 3, 2020

ghost Feb 3, 2020

ghost Feb 3, 2020

ghost Feb 3, 2020

tomgeorge Feb 3, 2020

ghost Feb 3, 2020

tomgeorge commented Feb 4, 2020 •

edited

Loading

tomgeorge commented Feb 4, 2020

ghost commented Feb 4, 2020

Add Script Support to Sidecars #1987

Add Script Support to Sidecars #1987

Conversation

tomgeorge commented Jan 30, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Jan 30, 2020

dibyom commented Jan 30, 2020

chmouel commented Jan 31, 2020

tomgeorge commented Jan 31, 2020

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Feb 3, 2020

tekton-robot commented Feb 3, 2020

ghost Feb 3, 2020

Choose a reason for hiding this comment

tomgeorge Feb 3, 2020

Choose a reason for hiding this comment

ghost Feb 3, 2020

Choose a reason for hiding this comment

ghost Feb 3, 2020

Choose a reason for hiding this comment

ghost Feb 3, 2020

Choose a reason for hiding this comment

tomgeorge Feb 3, 2020

Choose a reason for hiding this comment

ghost Feb 3, 2020

Choose a reason for hiding this comment

tomgeorge commented Feb 4, 2020 • edited Loading

tomgeorge commented Feb 4, 2020

ghost commented Feb 4, 2020

tomgeorge commented Jan 30, 2020 •

edited

Loading

tomgeorge commented Feb 4, 2020 •

edited

Loading