Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Improve the communication for automatic extension resolution #5082

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions internal/cmd/launcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func (l *launcher) launch(cmd *cobra.Command, args []string) error {
if err != nil {
l.gs.Logger.
WithError(err).
Error("Automatic extension resolution is enabled but it failed to analyze the dependencies." +
Error("Failed to analyze the extensions required." +
" Please, make sure to report this issue by opening a bug report.")
return err
}
Expand All @@ -103,8 +103,8 @@ func (l *launcher) launch(cmd *cobra.Command, args []string) error {

l.gs.Logger.
WithField("deps", deps).
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
WithField("deps", deps).
WithField("extensions", 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," +
Copy link
Contributor

@joanlopez joanlopez Aug 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Nit;

Suggested change
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

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some thoughts:

  1. I do think we should use extensions instead of dependancies 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
  2. 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.
  3. "custom binary" also makes it seems like we will make this specifically for the user, but it isn't.

I have made proposal below

Copy link
Contributor

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:

  1. we do not always download, sometimes we use a local copy
  2. 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

Copy link
Contributor Author

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.

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 do not like the wordign of "Provisioning"

@mstoykov same for provisioning. I guess it used to collect the two operations (build + download).

" may take a few seconds as it needs to download the new one.")
Comment on lines +106 to +107
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
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.


customBinary, err := l.provisioner.provision(deps)
if err != nil {
Expand Down
Loading