Update to k6provider v0.2.0 and update code #5313
Conversation
mstoykov
requested review from
AgnesToulet and
inancgumus
and removed request for
a team
mstoykov
temporarily deployed
to
azure-trusted-signing
GitHub Actions
Inactive
mstoykov
temporarily deployed
to
azure-trusted-signing
GitHub Actions
Inactive
internal/cmd/launcher.go
Outdated
| return err | ||
| } | ||
|
|
||
| deps := make(map[string]string, len(oldDeps)) |
There was a problem hiding this comment.
I think it is cleaner to make this transformation in analyze
internal/cmd/launcher.go
Outdated
| return true | ||
| } | ||
|
|
||
| // If dependency's constrain is null, assume it is "*" and consider it satisfied. |
There was a problem hiding this comment.
This comment should be moved to the place where this if fixed
internal/cmd/launcher.go
Outdated
|
|
||
| contraintsSemver, err := semver.NewConstraint(constraints) | ||
| if err != nil { | ||
| // Let it fail from the k6provider |
There was a problem hiding this comment.
I don't understand this comment. Why "fail from the k6provider"? If the constraint is not valid, why not fail here?
mstoykov
temporarily deployed
to
azure-trusted-signing
GitHub Actions
Inactive
mstoykov
temporarily deployed
to
azure-trusted-signing
GitHub Actions
Inactive
|
@pablochacin I started doing changes and realized we likely want to keep the smever constraints that are already parsed. Couldn't really figure out if I can have a constraint that is always true (aka the "*") so I dropped it for now. The future not k6deps code can likely figure more of this out and potentially fix the underlying problem. |
mstoykov
temporarily deployed
to
azure-trusted-signing
GitHub Actions
Inactive
mstoykov
temporarily deployed
to
azure-trusted-signing
GitHub Actions
Inactive
| " it's required to provision a custom binary.") | ||
|
|
||
| customBinary, err := l.provisioner.provision(deps) | ||
| customBinary, err := l.provisioner.provision(constraintsMapToProvisionDependancy(deps)) |
There was a problem hiding this comment.
| customBinary, err := l.provisioner.provision(constraintsMapToProvisionDependancy(deps)) | |
| customBinary, err := l.provisioner.provision(constraintsMapToProvisionDependency(deps)) |
Nit: TIL that both were correct but for consistency, it would be best to use the same spelling everywhere.
| return nil | ||
| } | ||
|
|
||
| func constraintsMapToProvisionDependancy(deps map[string]*semver.Constraints) k6provider.Dependencies { |
There was a problem hiding this comment.
| func constraintsMapToProvisionDependancy(deps map[string]*semver.Constraints) k6provider.Dependencies { | |
| func constraintsMapToProvisionDependency(deps map[string]*semver.Constraints) k6provider.Dependencies { |
| for name, constrain := range tc.deps { | ||
| dep, err := k6deps.NewDependency(name, constrain) |
There was a problem hiding this comment.
| for name, constrain := range tc.deps { | |
| dep, err := k6deps.NewDependency(name, constrain) | |
| for name, constraint := range tc.deps { | |
| dep, err := k6deps.NewDependency(name, constraint) |
Not from your PR so feel free to ignore it or update it in a future PR but in case you apply my other comments, you can apply this one as well.
There was a problem hiding this comment.
Also, for my own understanding, if the goal is to get rid of k6deps in the future, what do we plan for this function?
|
I will do this changes in a separate PR so I can rebase my other work on this post merge faster. Thanks for the comments Agnes 🙇 |
4 participants
What?
Update to k6provider v0.2.0.
Why?
Apart from updating the dependancy this, also now makes it not dependant on k6deps, which allows the future PR to drop it from k6.
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)