-
Notifications
You must be signed in to change notification settings - Fork 80
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
Rename execute to 'run' #720
base: main
Are you sure you want to change the base?
Conversation
b5280b5
to
f14801b
Compare
Signed-off-by: Michael Oviedo <[email protected]>
Signed-off-by: Michael Oviedo <[email protected]>
Signed-off-by: Michael Oviedo <[email protected]>
Signed-off-by: Michael Oviedo <[email protected]>
Signed-off-by: Michael Oviedo <[email protected]>
Signed-off-by: Michael Oviedo <[email protected]>
Signed-off-by: Michael Oviedo <[email protected]>
Signed-off-by: Michael Oviedo <[email protected]>
Signed-off-by: Michael Oviedo <[email protected]>
Signed-off-by: Michael Oviedo <[email protected]>
Signed-off-by: Michael Oviedo <[email protected]>
Signed-off-by: Michael Oviedo <[email protected]>
Signed-off-by: Michael Oviedo <[email protected]>
Signed-off-by: Michael Oviedo <[email protected]>
f14801b
to
831185d
Compare
scripts/analyze.py
Outdated
# stored in ~/.benchmark/benchmarks/test_executions/TEST_EXECUTION_TS/). | ||
# test_run.json files (it's a summary of the results of | ||
# a single test_run which is | ||
# stored in ~/.benchmark/benchmarks/test_runs/test_run_TS/). |
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 can be corrected: stored in ~/.benchmark/benchmarks/test_runs/<test_run_id>/)
osbenchmark/benchmark.py
Outdated
elif what == "test_executions": | ||
metrics.list_test_executions(cfg) | ||
test_run_orchestrator.list_pipelines() | ||
elif what == "test_runs": |
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.
How do you feel about switching this to test-runs
since it's front-facing and being invoked by the user in opensearch-benchmark list test_runs
? If we go with a hyphenated approach, we should also update the local directory where they are being stored as test-runs
directory.
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.
Personally, I'm on board with this. When I was first working with OSB, I'd often hit list test-executions
instead of list test_executions
. I think hyphens are more natural when it comes to running commands. I added this change for now but I think we should also get @gkamat opinion on this
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.
If we decide to go forth with this change, we would also have to do this for aggregated_results
but we could leave that for a separate PR..
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.
Left comments. We can sync up on the front-facing test_runs
to test-runs
if you'd like
Signed-off-by: Michael Oviedo <[email protected]>
Signed-off-by: Michael Oviedo <[email protected]>
Signed-off-by: Michael Oviedo <[email protected]>
Description
Renames instances of 'execute' to 'run', including 'execute-test' 'TestExecution*', etc. Please comment if I missed any instances!
Issues Resolved
#307
Testing
Tested with
make test
+make it
and by running test runs + aggregations with an OSB provisioned cluster + another 3 node cluster.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.