Integrate binary provisioning into k6 root command #4801

pablochacin · 2025-05-20T11:52:12Z

What?

integrates the logic of binary provisioning into the k6 root command.

Why?

Presently, the logic of binary provisioning happens before the k6 root command is executed. This brings some additional complexity and some problems because the arguments passed to k6 have not yet been parsed .

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)

Closes #4737
Closes #4758

Signed-off-by: Pablo Chacin <pablochacin@gmail.com>

codebien

The global architecture LGTM. There are a few minor adjustments required.

codebien · 2025-06-17T06:17:48Z

internal/cmd/root.go

+	return newRootWithLauncher(gs, newLauncher(gs))
+}
+
+// creates a root command with a launcher. Used to facilitate testing


Suggested change

// creates a root command with a launcher. Used to facilitate testing

// newRootWithLauncher creates a root command with a launcher. Used to facilitate testing

codebien · 2025-06-17T06:18:04Z

internal/cmd/root.go

 }

+// creates a root command with a default launcher


Suggested change

// creates a root command with a default launcher

// newRootCommand creates a root command with a default launcher

codebien · 2025-06-17T06:20:15Z

internal/cmd/launcher.go

+func isAnalysisRequired(cmd *cobra.Command) bool {
+	switch cmd.Name() {
+	case "run":
+		if cmd.Parent() != nil && cmd.Parent().Name() == "cloud" {


Suggested change

if cmd.Parent() != nil && cmd.Parent().Name() == "cloud" {

// exclude `k6 cloud run` command

if cmd.Parent() != nil && cmd.Parent().Name() == "cloud" {

codebien · 2025-06-17T06:26:47Z

internal/cmd/launcher.go

+// launch analyzies the command to be executed and its input (e.g. script)
+// to identify if its dependencies. It it has dependencies tha cannot be satisfied by the
+// current binary, it obtains a custom binary usign the provision function and delegates
+// the execution of the command to this binary. if not, continues with the execution of the
+// command in the current binary.


This comment needs some adjustment.

codebien · 2025-06-17T06:28:22Z

internal/cmd/launcher.go

-	// with their constrains, and it returns an executor that satisfies them.
-	provision func(*state.GlobalState, k6deps.Dependencies) (commandExecutor, error)
+// provider defines the interface for provisioning a custom k6 binary for a set of dependencies
+type provider interface {


Suggested change

type provider interface {

type provisioner interface {

It sounds to me more idiomatic. If we want provider then I expect a provide function.

codebien · 2025-06-17T06:31:31Z

internal/cmd/launcher_test.go


-			script := scriptNameFromArgs(tc.args)
+			// TODO: check error


Suggested change

// TODO: check error

// TODO: check error

Can we address this TODO, please?

it is a leftover. execute does not return any error

codebien · 2025-06-17T06:33:51Z

internal/cmd/launcher.go

-}
-
-// customBuildRequired checks if the build is required
+// isCustomBuildRequired checks if the build is required
 // it's required if there is one or more dependencies other than k6 itself
 // or if the required k6 version is not satisfied by the current binary's version
 // TODO: get the version of any built-in extension and check if they satisfy the dependencies


I have the feeling we have an issue for this TODO. Please, mention here directly the issue so they are bidirectionally linked.

Added. This isssue is addressed in this PR #4812

Signed-off-by: Pablo Chacin <pablochacin@gmail.com>

mstoykov · 2025-06-19T08:43:34Z

internal/cmd/launcher.go

+	// This can be avoided by checking if the current binary has already extensins that
+	// satisfies the dependencies. See comment in isCustomBuildRequired function.


Suggested change

// This can be avoided by checking if the current binary has already extensins that

// satisfies the dependencies. See comment in isCustomBuildRequired function.

// This can be avoided by checking if the current binary has already extensions that

// satisfies the dependencies. See comment in isCustomBuildRequired function.

Signed-off-by: Pablo Chacin <pablochacin@gmail.com>

mstoykov

I find the whole way the binary provisioning trying to be integrated ... strange.

In the sense that it isn't being integrated, it does practically the same thing as if it is a separate program, but now internally.

Looking at the problem it is trying to solve and given my lack of work with the previous work, I would not try to be wrapping the whole process and then try to parse stuff the same way as k6. Instead, I will let k6 parse everything it can and use that it does it the way it does to get the exact argument it has with all its other configurations to figure out if I need binary provisioning.

In this case IMO this means going to https://github.com/grafana/k6/blob/master/internal/cmd/test_load.go#L46 or even

k6/internal/cmd/test_load.go

Lines 177 to 186 in 34c2dc9

    
           func readSource(gs *state.GlobalState, filename string) (*loader.SourceData, map[string]fsext.Fs, string, error) { 
        
           	pwd, err := gs.Getwd() 
        
           	if err != nil { 
        
           		return nil, nil, "", err 
        
           	} 
        
           	filesystems := loader.CreateFilesystems(gs.FS) 
        
           	src, err := loader.ReadSource(gs.Logger, filename, pwd, filesystems, gs.Stdin) 
        
           	return src, filesystems, pwd, err 
        
           }

After this depending on the use case I guess you can error out with a specific error of "We need this modules" and then capturing it potentially in a similar way in the root command.

This IMO not only will reduce the code here, but will also be way more future prove. As you won't need to try to work around you not having parsed some configuration or us having anew command that reads the source code that needs to be wrapped.

I looked at some of the other changes in the other PRs, but it doesn't seem like any of them stop trying to parse the configuration as k6, just to do it again.

Is there a reason this was not considered or are there any known problems with something like this that I do not know about ?

pablochacin · 2025-06-19T12:33:42Z

@mstoykov

In the sense that it isn't being integrated, it does practically the same thing as if it is a separate program, but now internally

I think part of this impression may come from the fact that I split the integration into two PRs (this and #4811)) to reduce the scope of review.

The difference of this approach from having an external program (which is basically how we did the initial "integration" as a wrapper of k6 command), is that now we rely on k6 command logic for parsing arguments and to read the source (see #4811)

Is there a reason this was not considered or are there any known problems with something like this that I do not know about ?

Basically, to have a clear execution path: either with binary provisioning or without it. See my comment below about why I considered this an advantage.

After this depending on the use case I guess you can error out with a specific error of "We need this modules" and then capturing it potentially in a similar way in the root command.

I'm not sure about this option. It depends on the command not having side-effects, and therefore stopping its execution and then retrying with binary provisioning will not find these side-effects. I find it harder to reason about this execution logic that the one existing now.

Also, I'm not sure how k6 handles importing files. From what I saw in the readSource we are only reading the "main" test script. What happens if it imports other files? can we analyze all the import tree before starting execution?

This IMO not only will reduce the code here, but will also be way more future prove. As you won't need to try to work around you not having parsed some configuration or us having anew command that reads the source code that needs to be wrapped.

I looked at some of the other changes in the other PRs, but it doesn't seem like any of them stop trying to parse the configuration as k6, just to do it again.

Not sure what duplicated parsing you refer to? There's no additional parsing, unless you mean parsing the test code itself searching dependencies. Can you give an example of this duplicated parsing? (again, consider this in the context of #4811.

In general, I'm not against a deeper integration, but maybe only if we ensure this readSource function is executed earlier in the command execution to avoid side-effects and we can decide which way to follow.

pablochacin mentioned this pull request May 20, 2025

WIP: integrate binary provisioning into root command #4689

Closed

8 tasks

pablochacin force-pushed the binary-provisioning/integrate-root-cmd branch 2 times, most recently from 0e9b32c to 69c540b Compare May 20, 2025 15:59

pablochacin changed the title ~~initial integration~~ Integrate binary provisioning into k6 root command May 20, 2025

pablochacin force-pushed the binary-provisioning/integrate-root-cmd branch 2 times, most recently from fd5f482 to cd6b16a Compare May 22, 2025 15:42

pablochacin added 2 commits May 22, 2025 19:01

initial integration

e6564f6

Signed-off-by: Pablo Chacin <pablochacin@gmail.com>

refactor provisioning into an interface

b1ba5b7

Signed-off-by: Pablo Chacin <pablochacin@gmail.com>

pablochacin force-pushed the binary-provisioning/integrate-root-cmd branch 2 times, most recently from e6edf2e to b1ba5b7 Compare May 23, 2025 07:44

pablochacin marked this pull request as ready for review May 23, 2025 11:00

pablochacin requested a review from a team as a code owner May 23, 2025 11:00

pablochacin requested review from mstoykov and codebien and removed request for a team May 23, 2025 11:00

This was referenced May 23, 2025

analyze built-in extensions #4812

Open

Binary provisioning/use loadsource #4811

Open

codebien requested changes Jun 17, 2025

View reviewed changes

pablochacin added 4 commits June 18, 2025 22:33

attend review comments

da4342e

Signed-off-by: Pablo Chacin <pablochacin@gmail.com>

add reference to issue

89cf479

Signed-off-by: Pablo Chacin <pablochacin@gmail.com>

Merge branch 'master' into binary-provisioning/integrate-root-cmd

f3030f7

attend linter issue

f66ae6a

Signed-off-by: Pablo Chacin <pablochacin@gmail.com>

mstoykov reviewed Jun 19, 2025

View reviewed changes

fix type in comment

afd8b69

Signed-off-by: Pablo Chacin <pablochacin@gmail.com>

mstoykov reviewed Jun 19, 2025

View reviewed changes

pablochacin requested a review from codebien June 19, 2025 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Integrate binary provisioning into k6 root command #4801

Integrate binary provisioning into k6 root command #4801

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	// creates a root command with a launcher. Used to facilitate testing
	// newRootWithLauncher creates a root command with a launcher. Used to facilitate testing

	// creates a root command with a default launcher
	// newRootCommand creates a root command with a default launcher

	if cmd.Parent() != nil && cmd.Parent().Name() == "cloud" {
	// exclude `k6 cloud run` command
	if cmd.Parent() != nil && cmd.Parent().Name() == "cloud" {

		// This can be avoided by checking if the current binary has already extensins that
		// satisfies the dependencies. See comment in isCustomBuildRequired function.

	func readSource(gs state.GlobalState, filename string) (loader.SourceData, map[string]fsext.Fs, string, error) {
	pwd, err := gs.Getwd()
	if err != nil {
	return nil, nil, "", err
	}

	filesystems := loader.CreateFilesystems(gs.FS)
	src, err := loader.ReadSource(gs.Logger, filename, pwd, filesystems, gs.Stdin)
	return src, filesystems, pwd, err
	}

Integrate binary provisioning into k6 root command #4801

Are you sure you want to change the base?

Integrate binary provisioning into k6 root command #4801

Uh oh!

Conversation

Uh oh!

What?

Why?

Checklist

Checklist: Documentation (only for k6 maintainers and if relevant)

Related PR(s)/Issue(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!