Skip to content
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

Fix some CLI UX issues #5

Merged
merged 4 commits into from
Dec 2, 2020
Merged

Fix some CLI UX issues #5

merged 4 commits into from
Dec 2, 2020

Conversation

imiric
Copy link
Contributor

@imiric imiric commented Dec 1, 2020

This closes #1, addresses the PATH issue mentioned in #2 (comment), fixes go list -m all for modules with vendored dependencies, and does some README cleanup. It's easier to review by commit, as I didn't want to create 4 separate PRs :)

We don't have automated tests for this (maybe we should?), but I confirmed it working as expected locally.

@imiric imiric requested review from mstoykov and na-- December 1, 2020 12:56
@@ -175,7 +182,7 @@ func runDev(ctx context.Context, args []string) error {
// and since this tool is a carry-through for the user's actual
// go.mod, we need to transfer their replace directives through
// to the one we're making
cmd = exec.Command("go", "list", "-m", "-f={{if .Replace}}{{.Path}} => {{.Replace}}{{end}}", "all")
cmd = exec.Command("go", "list", "-mod=readonly", "-m", "-f={{if .Replace}}{{.Path}} => {{.Replace}}{{end}}", "all")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this can be considered a fix, but at least it doesn't fail when run from a vendored module directory.

@@ -94,11 +96,11 @@ The race detector can be enabled by setting `XK6_RACE_DETECTOR=1`.

```go
builder := xk6.Builder{
k6Version: "v2.0.0",
k6Version: "v0.29.0",
Plugins: []xk6.Dependency{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of mention of "plugins" still, but not sure if we should bother with converting them all to "extensions".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am for s/plugin/extension/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, let's do this in #7.

Comment on lines -142 to +151
return "k6.exe"
return ".\\k6.exe"
}
return "k6"
return "./k6"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am more inclined to have runDev use a temporary place for the k6 binary instead of overwriting the one in the current directory and then ... possibly deleting it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I slightly disagree. I get the overwriting/deletion concern, but this is meant to help with quick dev iterations when run from an extension repo, and it's much easier to have the binary in the CWD than in a temp dir somewhere... If you care about where it's written, you should use xk6 build --output instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But ... you delete the file after that ... unless an env variable is set - so in the majority of the cases you will just delete the file 'k6' in the local directory, nothing else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, and if XK6_SKIP_CLEANUP is set then I'll have to hunt down the binary in a temp dir, which is more annoying than it overwriting/deleting ./k6, that should've been produced by a previous xk6 run anyway.

@na-- Tie breaker? 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The path is literally a few lines above
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can resolve this later :)

Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I left 2 nitpicks

@imiric imiric merged commit d7e4293 into master Dec 2, 2020
@imiric imiric deleted the fix/cli-ux branch December 2, 2020 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using . for replace should work
3 participants