Skip to content

Conversation

codebien
Copy link
Contributor

@codebien codebien commented Aug 22, 2025

What & Why?

  1. Now that automatic extension resolution is the default doesn't make sense to repeat it for each logline, this should be obvious as it is the default.
  2. The current waiting time on a blinking prompt is not optimal to me, it doesn't seem obvious to me if I should wait or the program got stuck so I clarified this aspect in the message.

Checklist

  • I have performed a self-review of my code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added tests for my changes.
  • I have run linter and tests locally (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.

  • I have added the correct milestone and labels to the PR.
  • I have updated the release notes: link
  • I have updated or added an issue to the k6-documentation: grafana/k6-docs#NUMBER if applicable
  • I have updated or added an issue to the TypeScript definitions: grafana/k6-DefinitelyTyped#NUMBER if applicable

Related PR(s)/Issue(s)

@codebien codebien added this to the v1.3.0 milestone Aug 22, 2025
@codebien codebien self-assigned this Aug 22, 2025
@codebien codebien requested a review from a team as a code owner August 22, 2025 10:22
@codebien codebien requested review from mstoykov and joanlopez and removed request for a team August 22, 2025 10:22
@@ -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," +
Copy link
Contributor

@joanlopez joanlopez Aug 22, 2025

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).

Comment on lines +106 to +107
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.")
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.

@@ -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," +
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

@@ -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).

@codebien codebien modified the milestones: v1.3.0, v1.4.0 Sep 11, 2025
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.

3 participants