-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Improve the communication for automatic extension resolution #5082
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
base: master
Are you sure you want to change the base?
Conversation
@@ -103,8 +103,8 @@ func (l *launcher) launch(cmd *cobra.Command, args []string) error { | |||
|
|||
l.gs.Logger. | |||
WithField("deps", deps). | |||
Info("Automatic extension resolution is enabled. The current k6 binary doesn't satisfy all dependencies," + | |||
" it's required to provision a custom binary.") | |||
Info("Provisioning a custom binary as the current doesn't satisfy all dependencies," + |
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.
Nit;
Info("Provisioning a custom binary as the current doesn't satisfy all dependencies," + | |
Info("Provisioning a custom binary because the current one doesn't satisfy all the dependencies," + |
I think it's slightly more precise and clear
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.
Overall, I'm wondering whether we should use either extensions
or dependencies
, and if we should be consistent with that use in the two messages modified in this 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.
Some thoughts:
- I do think we should use
extensions
instead ofdependancies
as the secodn can and likely will be misunderstood as./somelib.js
Or even worse something else that needs to be download such as chromium or a c library or whatever - I do not like the wordign of "Provisioning" - we will be downloading a binary. I think we should jsut use download instead of the internal term.
- "custom binary" also makes it seems like we will make this specifically for the user, but it isn't.
I have made proposal below
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 guess after the discussion now nad looking more closely at some of the code:
- we do not always download, sometimes we use a local copy
- we do not log anything in the above cases at any log level
I do reall think it will be nice to one differntiate between those two case and potentially list both the binary link and potentially the binary path on the fs. Maybe not at Info but debug level
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.
Overall, I'm wondering whether we should use either extensions or dependencies
I guess the word dependencies
here has been used because there is k6 itself to be resolved which is not an extensions.
I will look deeper into a better proposal.
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 do not like the wordign of "Provisioning"
@mstoykov same for provisioning. I guess it used to collect the two operations (build + download).
Info("Provisioning a custom binary as the current doesn't satisfy all dependencies," + | ||
" may take a few seconds as it needs to download the new one.") |
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.
Info("Provisioning a custom binary as the current doesn't satisfy all dependencies," + | |
" may take a few seconds as it needs to download the new one.") | |
Info("Downloading a binary as the current isn't build with all required extensions," + | |
" may take a few seconds.") |
The current listing of deps
also IMO needs help.
@@ -103,8 +103,8 @@ func (l *launcher) launch(cmd *cobra.Command, args []string) error { | |||
|
|||
l.gs.Logger. | |||
WithField("deps", deps). | |||
Info("Automatic extension resolution is enabled. The current k6 binary doesn't satisfy all dependencies," + | |||
" it's required to provision a custom binary.") | |||
Info("Provisioning a custom binary as the current doesn't satisfy all dependencies," + |
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.
Some thoughts:
- I do think we should use
extensions
instead ofdependancies
as the secodn can and likely will be misunderstood as./somelib.js
Or even worse something else that needs to be download such as chromium or a c library or whatever - I do not like the wordign of "Provisioning" - we will be downloading a binary. I think we should jsut use download instead of the internal term.
- "custom binary" also makes it seems like we will make this specifically for the user, but it isn't.
I have made proposal below
@@ -103,8 +103,8 @@ func (l *launcher) launch(cmd *cobra.Command, args []string) error { | |||
|
|||
l.gs.Logger. | |||
WithField("deps", deps). |
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.
WithField("deps", deps). | |
WithField("extensions", deps). |
What & Why?
Checklist
make check
) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)