Binary provisioning/use loadsource #4811
Conversation
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
e6edf2e to
9066d48
Compare
internal/cmd/launcher.go
Outdated
| Debug("The command did not receive an input script.") | ||
| return nil, errScriptNotFound | ||
| if len(args) < 1 { | ||
| return nil, fmt.Errorf("k6 needs at least one argument to load the test") |
There was a problem hiding this comment.
| return nil, fmt.Errorf("k6 needs at least one argument to load the test") | |
| return nil, fmt.Errorf("the invoked command needs a file path or pass the test via Stdin") |
This message sounds too vague. What argument do we expect? I guess we can replace k6 with something clarifies better where is the issue. I suggested invoked command but probably there is some better.
internal/cmd/launcher.go
Outdated
| @@ -277,37 +276,37 @@ func extractToken(gs *state.GlobalState) (string, error) { | |||
| // Presently, only the k6 input script or archive (if any) is passed to k6deps for scanning. | |||
| // TODO: if k6 receives the input from stdin, it is not used for scanning because we don't know | |||
| // if it is a script or an archive | |||
| func analyze(gs *state.GlobalState, args []string) (k6deps.Dependencies, error) { | |||
| func analyze(gs *state.GlobalState, _ *cobra.Command, args []string) (k6deps.Dependencies, error) { | |||
There was a problem hiding this comment.
| func analyze(gs *state.GlobalState, _ *cobra.Command, args []string) (k6deps.Dependencies, error) { | |
| func analyze(gs *state.GlobalState, args []string) (k6deps.Dependencies, error) { |
I don't like the fact we are passing Cobra primitives around. Is it really required to have this dependency? Can we break it in Go/k6 primitives and more scoped for the function?
There was a problem hiding this comment.
I'm not sure what the problem is with using Cobra types, as we are within the execution of a Cobra command. Can you elaborate on what problems you see?
And, I'm not sure what you mean by "break it in Go/k6 primitives". Create abstractions around Cobra?
internal/cmd/launcher.go
Outdated
| Debug("Test script provided by Stdin is not yet supported from Binary provisioning feature.") | ||
| return nil, errUnsupportedFeature | ||
| sourceRootPath := args[0] | ||
| gs.Logger.Debugf("Resolving and reading test '%s'...", sourceRootPath) |
There was a problem hiding this comment.
| gs.Logger.Debugf("Resolving and reading test '%s'...", sourceRootPath) | |
| gs.Logger.WithField("source", "sourceRootPath).Debug("Launcher is resolving and reading the test") |
Now we have multiple components doing this reading operation, could be useful to explicit Who / Where the operation is executed.
internal/cmd/launcher.go
Outdated
| gs.Logger.Debugf("Resolving and reading test '%s'...", sourceRootPath) | ||
| src, _, pwd, err := readSource(gs, sourceRootPath) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
| return nil, err | |
| return nil, fmt.Errorf(err |
We need to wrap this error and make it clear the error's path.
internal/cmd/launcher_test.go
Outdated
| ts.CmdArgs = k6Args | ||
|
|
||
| // k6deps uses os package to access files. So we need to use it in the global state | ||
| ts.FS = afero.NewOsFs() |
There was a problem hiding this comment.
| ts.FS = afero.NewOsFs() | |
| ts.FS = fsext.NewMemMapFS() |
Is the afero dependency really needed?
| return nil, errUnsupportedFeature | ||
| sourceRootPath := args[0] | ||
| gs.Logger.Debugf("Resolving and reading test '%s'...", sourceRootPath) | ||
| src, _, pwd, err := readSource(gs, sourceRootPath) |
There was a problem hiding this comment.
I don't like the idea to have this function called twice. Can we move this function as early as possible to be good for the entire k6 program? Independently if Binary Provisioning is enabled or not. In this way, the part after can assume it is always already loaded.
I don't know if it is really possible, but I want to make sure that we thought about it and we have reasons if we have to call it twice.
There was a problem hiding this comment.
I will open another PR trying to address this particular issue as it probably will touch many places in the code inderectly.
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
|
👀 |
internal/cmd/launcher.go
Outdated
| // is we are here, it is possible that the analyze function read the stdin to analyse it | ||
| // and restored the content into gs.Stdin, so we pass it to the command |
There was a problem hiding this comment.
This comment needs a re-arrangement.
There was a problem hiding this comment.
Not sure what you refer to, but the initial "is" should be "if".
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
What?
Use the loadsource function for dependency analysis
Why?
Presently, binary provisioning uses its own logic for loading the input for dependency analysis. This logic has some shortcomings like not handling input for stdin or limiting the processing of scripts with spe 10000 cific extensions.
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)
Depens on #4801
Closes #4727
Closes #4696