-
Notifications
You must be signed in to change notification settings - Fork 314
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
Option track-revision should work with track-repository #751
Conversation
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.
Thanks for the PR!
This works fine and all tests seem to pass.
I left a request to add an integration test to ensure we catch future regressions.
@@ -191,7 +191,7 @@ def positive_number(v): | |||
track_source_group.add_argument( | |||
"--track-path", | |||
help="Define the path to a track.") | |||
track_source_group.add_argument( | |||
p.add_argument( |
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.
Could we add an integration test?
We could use the list command:
Lines 219 to 233 in 966891a
function test_list { | |
local cfg | |
random_configuration cfg | |
info "test list races [${cfg}]" | |
esrally list races --configuration-name="${cfg}" | |
info "test list cars [${cfg}]" | |
esrally list cars --configuration-name="${cfg}" | |
info "test list Elasticsearch plugins [${cfg}]" | |
esrally list elasticsearch-plugins --configuration-name="${cfg}" | |
info "test list tracks [${cfg}]" | |
esrally list tracks --configuration-name="${cfg}" | |
info "test list telemetry [${cfg}]" | |
esrally list telemetry --configuration-name="${cfg}" | |
} |
and, in order to avoid having to add the eventdata
repo in the rally.ini used by the integration script (which is randomized btw), we could just rely on the default repository with something like:
info "test list can use track revision together with track repository"
esrally list tracks --configuration-name="${cfg}" --track-repository=default --track-revision=4080dc9850d07e23b6fc7cfcdc7cf57b14e5168d
}
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.
Sure I will add it to the integration tests.
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.
LGTM thanks
Option track-revision should work with track-repository but not track-path
Both options track-repository and track-revision should work together but not with track-path option.
Fixes: #740