-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
roachtest: pass RunOptions instead of NodeListOption #117320
roachtest: pass RunOptions instead of NodeListOption #117320
Conversation
9d421fc
to
5f71972
Compare
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @renatolabs and @srosenberg)
pkg/cmd/roachtest/option/options.go
Outdated
@@ -80,3 +80,8 @@ type StopOpts struct { | |||
func DefaultStopOpts() StopOpts { | |||
return StopOpts{RoachprodOpts: roachprod.DefaultStopOpts()} | |||
} | |||
|
|||
// OnNodes returns a RunOptions that will run on the given nodes. | |||
func OnNodes(nodes NodeListOption) install.RunOptions { |
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.
Nit: WithNodes
seems more standard
@@ -101,11 +101,11 @@ type Cluster interface { | |||
|
|||
// Run is fatal on errors. | |||
// Use it when an error means the test should fail. | |||
Run(ctx context.Context, node option.NodeListOption, args ...string) | |||
Run(ctx context.Context, options install.RunOptions, args ...string) | |||
|
|||
// RunE runs a command on the specified nodes and returns an error. |
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.
Nit: might be worth expanding the comment; e.g.,
... on the specified nodes (
RunOptions.Nodes
) with specifiedRetryOptions
...
5f71972
to
9554c8b
Compare
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @renatolabs)
pkg/cmd/roachtest/cluster/cluster_interface.go
line 106 at r1 (raw file):
Previously, srosenberg (Stan Rosenberg) wrote…
Nit: might be worth expanding the comment; e.g.,
... on the specified nodes (
RunOptions.Nodes
) with specifiedRetryOptions
...
Good idea, Done.
pkg/cmd/roachtest/option/options.go
line 85 at r1 (raw file):
Previously, srosenberg (Stan Rosenberg) wrote…
Nit:
WithNodes
seems more standard
Makes sense, I'll update the roachprod side toWithNodes
as well for consistency.
adc8861
to
ee81105
Compare
Previously, OnNodes was added to roachprod as a method to easily create RunOptions that will run on the specified nodes. This function has been renamed to WithNodes to be more aligned with the other RunOption methods. Additionally, a convenience function, WithNodes, has been added to roachtest options to instantiate RunOptions with a NodeListOption. Epic: None Release Note: None
This change updates the `Run` and `RunE` cluster interface methods to take `install.RunOptions` as a parameter instead of `option.NodeListOption` to expose the additional available options to `roachtest`. Epic: None Release Note: None
ee81105
to
34151d5
Compare
TFTR! bors r=srosenberg |
Build succeeded: |
This change updates the
Run
andRunE
cluster interface methods to takeinstall.RunOptions
as a parameter instead ofoption.NodeListOption
to expose the additional available options toroachtest
.Epic: None
Release Note: None