Shell package cleanup #3068
Merged
Shell package cleanup #3068
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DrJosh9000
force-pushed
the
pipe-358-shell-refactor
branch
12 times, most recently
from
fc3548c to
6f4fa88
Compare
DrJosh9000
force-pushed
the
pipe-358-shell-refactor
branch
10 times, most recently
from
2ca681e to
9caf9c1
Compare
Using `atomic.Pointer` instead of `sync.Mutex`, and only storing the `process.Process`, since the `process.Config` isn't needed after start.
Add dry run and command log options
Rename and unexport Shell.Writer -> Shell.stdout
Note that RunAndCaptureStdout has a different opinion of what to do with stderr: it is now reflected in the output.
DrJosh9000
force-pushed
the
pipe-358-shell-refactor
branch
from
9caf9c1 to
aace2b6
Compare
wolfeidau
approved these changes
Nov 7, 2024
| @@ -347,6 +347,10 @@ func (p *Process) Started() <-chan struct{} { | |||
|
|
|||
| // Interrupt the process on platforms that support it, terminate otherwise | |||
| func (p *Process) Interrupt() error { | |||
| if p == nil { | |||
There was a problem hiding this comment.
You can call a function on a nil Process?
There was a problem hiding this comment.
Under the hood p is just another argument to a function. (*Process)(nil) still has the type *Process, so Go will find the method and call it.
|
I'm going to merge this now rather than later:
|
This was referenced Nov 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The
shellpackage is going to be used outside the job executor, so it should be moved up one level and also tidied up substantially.Changes
There's a lot happening here so it's in several commits.
Each one is fairly focused on a particular aspect, so shouldn't be too maddening to review.
One thing that's worth calling out: this changes how
RunAndCapture(now calledRunAndCaptureStdout, on a different type) handlesstderr. Previously,stderrof the process being run was dropped with no way to override it. I think that's not very useful, so I changed the default behaviour to showstderr(with an override via functional options).Testing
go test ./...). Buildkite employees may check this if the pipeline has run automatically.go fmt ./...)