-
Notifications
You must be signed in to change notification settings - Fork 224
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
Fixed docker command in get started doc #1838
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.
@atilbian thanks for opening the PR! It looks good to me, I'll wait for one of the other reviews to take a look as well before merging this. 🙇
@@ -33,7 +33,7 @@ To run a simple local script: | |||
``` | |||
|
|||
```docker | |||
$ docker run --rm -i -v $PWD:/app -w /app grafana/k6 new | |||
$ docker run --rm -i --user $UID -v $PWD:/app -w /app grafana/k6 new |
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.
$ docker run --rm -i --user $UID -v $PWD:/app -w /app grafana/k6 new | |
$ docker run --rm -i --user $UID -v $PWD:/app -w /app grafana/k6 new |
This is bash dependent, should we use instead $(id -u)
? @oleiade Does it work for you on fish?
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.
Good point. In fish I have to quote "$ENV_VARIABLE", but --user "$UID"
does work. Let's not bother too much about fish because it has its set of differences with standard shells like $(id -u)
would be (id -u)
anyway.
I do think switching to $(id -u)
to be a good idea nonetheless anyway 👍🏻 (fish users will know what to do about it 🙇🏻
Hey @atilbian,
it includes the following improvements:
|
@atilbian thanks for your contribution 🙇 |
What?
It fixes the docker command listed in the
Get started documentation
following #1536Checklist
npm start
command locally and verified that the changes look good.docs/sources/k6/next
folder of the documentation.docs/sources/k6/v{most_recent_release}
folder of the documentation.Related PR(s)/Issue(s)
k6 new
command when used in the context of Docker #1536