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

Warn users on importing dependencies as absolute paths #2078

Merged
merged 5 commits into from
Jul 8, 2021

Conversation

mstoykov
Copy link
Contributor

part of #1089

@codecov-commenter
Copy link

codecov-commenter commented Jun 25, 2021

Codecov Report

Merging #2078 (459c426) into master (b232e97) will decrease coverage by 0.07%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2078      +/-   ##
==========================================
- Coverage   72.16%   72.08%   -0.08%     
==========================================
  Files         181      180       -1     
  Lines       14289    14248      -41     
==========================================
- Hits        10311    10271      -40     
  Misses       3351     3351              
+ Partials      627      626       -1     
Flag Coverage Δ
ubuntu 72.01% <0.00%> (-0.08%) ⬇️
windows 71.73% <0.00%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
js/initcontext.go 87.87% <0.00%> (-4.68%) ⬇️
lib/testutils/httpmultibin/httpmultibin.go 90.00% <0.00%> (-1.12%) ⬇️
lib/executor/vu_handle.go 94.39% <0.00%> (-0.94%) ⬇️
cmd/outputs.go 0.00% <0.00%> (ø)
output/statsd/datadog.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b232e97...459c426. Read the comment docs.

Comment on lines 157 to 158
i.logger.Warnf("you import '%s' with an absolute path, this won't be cross platform and likely will not work if"+
" you move the script between machines. Also it might not work on k6 cloud if it doesn't have `file://` in front",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
i.logger.Warnf("you import '%s' with an absolute path, this won't be cross platform and likely will not work if"+
" you move the script between machines. Also it might not work on k6 cloud if it doesn't have `file://` in front",
i.logger.Warnf("you import '%s' with an absolute path, this won't be cross platform and likely will not work if"+
" you move the script between machines or in cloud. You should use `file://` in front to have the full cross platform and cloud finally supported",

I would make "the suggestion" more explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is that "full cross platform" isn't true, as just moving the script between machines will still not work. It will work if you archive it and run the archive, but not if you than unarchive it on a different machine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reality of it is that the correct way for something to be cross-platform and cross-machine (as the OS is not the only problem) is to not use absolute paths at all.

So I really do prefer if that is the sentiment we sent more than "this won't work in the cloud if you are on Windows and use absolute paths without file://"

cc @na-- @imiric

Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

👎, this is going to trigger for *nix file paths, which are not problematic. Also, the warning will be shown multiple times, once for every VU init.

@mstoykov
Copy link
Contributor Author

this is going to trigger for Linux file paths,

They still are not

  1. cross platform
  2. cross machine in most cases, even if k6 archive + k6 run archive.tar on different machines will work (hopefully)

@na--
Copy link
Member

na-- commented Jun 25, 2021

See my other point about the warning being shown once per VU, I submitted my review prematurely and edited the message. Also, if you want this warning to be for all absolute paths (which I don't), it should also be shown for file:// imports, which it isn't.

@mstoykov mstoykov requested review from codebien and na-- July 1, 2021 08:12
js/initcontext.go Outdated Show resolved Hide resolved
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

LGTM in general, made an inline comment with a suggestion for a better warning message

Co-authored-by: na-- <n@andreev.sh>
codebien
codebien previously approved these changes Jul 7, 2021
@mstoykov mstoykov merged commit c2e19d9 into master Jul 8, 2021
@mstoykov mstoykov deleted the addWarningForAbsolutePath branch July 8, 2021 06:53
@na-- na-- added this to the v0.34.0 milestone Jul 8, 2021
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.

4 participants