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

Add choice of "command interposer" #371

Merged
merged 5 commits into from
Aug 28, 2024
Merged

Add choice of "command interposer" #371

merged 5 commits into from
Aug 28, 2024

Conversation

DrJosh9000
Copy link
Contributor

@DrJosh9000 DrJosh9000 commented Aug 26, 2024

What

Change how podSpec/command and podSpec/args is interpreted. The choice of "interposer" is up to the user. If they allow the Kubernetes plugin, the choice can be made per-step, otherwise there is a default that can be set in values.yaml.

The default (if no choice is made) is changing.

buildkite mode is more convenient if you are used to pipeline.yaml syntax, and want to pass multiple commands as a list. vector mode is much more like how Kubernetes treats command and args, and is intended to execute a single command (that, per the Kubernetes docs, can be ["sh", "-c", "big script here"]. There's also legacy mode, equivalent to what it does before this PR, to ease transitioning pipelines over to new modes.

Why

Joining the container command and args with strings.Join(..., " ") was never exactly a good idea.

For example, before this PR, this doesn't do what it looks like it does:

podSpec:
  containers:
    - command:
        - echo First command
        - echo Second command

Because each command component was joined with spaces, this would run the command echo First command echo Second command, and echo First command echo Second command, not two things over two lines.

If we tried harder to interpret it the way Kubernetes does (as a raw arg vector executed directly, not a string interpreted by a shell), then we should be using the vector ["echo First command", "echo Second command"], which would fail because while echo could be an executable file (that takes one or more arguments), it's unlikely that echo First command is the name of an executable.

With no apologies to Terry Pratchet: If we're breaking Kubernetes' rules, we ought to break 'em good an' 'ard.

@DrJosh9000 DrJosh9000 force-pushed the choice-of-interposer branch 8 times, most recently from 780cf82 to 30feb11 Compare August 27, 2024 07:16
@DrJosh9000 DrJosh9000 force-pushed the choice-of-interposer branch 2 times, most recently from f7c7968 to 290681e Compare August 27, 2024 07:45
@DrJosh9000 DrJosh9000 requested a review from a team August 27, 2024 07:46
@DrJosh9000 DrJosh9000 force-pushed the choice-of-interposer branch 13 times, most recently from d5a2368 to 9601fa7 Compare August 28, 2024 01:32
@wolfeidau
Copy link
Contributor

@DrJosh9000 not a fan of the readme changes, was there a reason for this?

Happy to defer to others but without some sort of automated formatter this feels like it is just adding friction.

@DrJosh9000
Copy link
Contributor Author

@wolfeidau it's not important here, so I'll back out the 100 column change.

Copy link
Contributor

@wolfeidau wolfeidau left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏻 🚀

@DrJosh9000 DrJosh9000 merged commit a36ac79 into main Aug 28, 2024
1 check passed
@DrJosh9000 DrJosh9000 deleted the choice-of-interposer branch August 28, 2024 05:56
DrJosh9000 added a commit that referenced this pull request Sep 2, 2024
DrJosh9000 added a commit that referenced this pull request Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants