-
Notifications
You must be signed in to change notification settings - Fork 8
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
Migrate Command Runner Constructors to Use Config Objects #269
Conversation
@@ -38,12 +38,21 @@ func (r IPTables) ID() string { | |||
|
|||
func (r IPTables) Run() op.Op { | |||
startTime := time.Now() | |||
if r.OS == "linux" { | |||
if r.OS != "linux" { |
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.
Unrelated to the change at hand, but this looked like a little bug that had crept in as part of a recent merge.
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.
Yeah, this could work -- it's certainly a nicer interface for people who aren't intimately familiar with our runners. My kingdom for named arguments in Go...
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.
This looks good! My only review item is nonblocking: we should lean on "string" as the default Format, like hcl, and elide the field in cfg when we're not overriding it.
This merge updates the Command constructors to leverage a new CommandConfig object, rather than parameters. The intention is to simplify future feature additions by limiting changes to the function signatures of these constructors.