-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix and clarify tests READMEs #190
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
Conversation
tests/smoke/README.md
Outdated
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.
The -cluster flag still exists, although it looks like it should be -cluster and not --cluster.
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.
I thought it was repeated here is all. I was thinking we could get rid of the extra explanation of the --cluster flag (I've been using --cluster, not -cluster), bc isn't it explained below, too? maybe I'm wrong, I'll take another look.
tests/smoke/README.md
Outdated
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 don't actually need to build tarball or run this tar command, do you?
+1 on the tests/smoke -> smoke_tests change. Maybe I'd broke that in #143.
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.
I'll check, maybe not
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.
yes, removed tarball and tar -zxf not required
tests/smoke/README.md
Outdated
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.
The -test.v flag still exists, but again, it looks like it only takes one dash.
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.
I've edited to take only 1 dash, but 2 dashes also still works lol
tests/smoke/README.md
Outdated
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.
Nice. I'd missed this in #99.
tests/smoke/README.md
Outdated
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.
Ah, you're just consolidating the above flag examples. I'm fine with either the old way or your new consolidation there. Wherever they end up, we'll want the single-dash forms.
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.
ok! It felt redundant to me, as I was following along, but I'm also ok with either. The double dash 'works' because that's what I've been using, I didn't realize the single-dash was a possibility. I'll change to the single, confirm they work, also.
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.
edited all to have the single dash
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sallyom, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR adds a bit of clarity when running smoke tests (locally) against an already running cluster, as opposed to running tests/run.sh to launch a cluster for you (Does anyone use that?).