-
Notifications
You must be signed in to change notification settings - Fork 5
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 mockable executor #150
base: main
Are you sure you want to change the base?
Conversation
This patch introduces a layer over os.exec that makes it possible to write testable code that doesn't run actual commands. The common executor interface is acompanied by two implementations, on that runs the commands normally (as the previous code did), and a "Dry Run" executor that keeps track of what would be executed. This enables us to have a "dry-run" mode that instead of executing the commands writes down what would be run, making it easy to run the same commands by hand when debugging. See the unit tests for more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You didn't replace the RunSystemCmd*
stuff in create_srpm_from_git.go
. Is that intentional ?
} | ||
|
||
// Join strings in a way that preserves the shell token boundries. For instance | ||
// the args ["cat", "a file"] when simpy joined with strings. Join would result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typos:
simpy -> simply
strings. Join -> strings.Join
} | ||
|
||
func (ex *DryRunExecutor) GenerateShellScript() string { | ||
preambule := "#!/usr/bin/env sh\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: preambule -> preamble
if dir == "" { | ||
dir = "$PWD" | ||
} | ||
full_invocation := fmt.Sprintf("(cd '%s' && %s)", dir, escapedInvocation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't cwd be restored ?
This patch introduces a layer over os.exec that makes it possible to write testable code that doesn't run actual commands. The common executor interface is acompanied by two implementations, on that runs the commands normally (as the previous code did), and a "Dry Run" executor that keeps track of what would be executed. This enables us to have a "dry-run" mode that instead of executing the commands writes down what would be run, making it easy to run the same commands by hand when debugging.
See the unit tests for more details.