-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add deps command to return dependencies of a given script/archive #5410
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.
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
base: master
Are you sure you want to change the base?
Conversation
This does include dependancies that might be needed for building a new binary with auto extension resolution, but also all imports. Similar to auto extension resolution, this does take into account only imports not `require` calls. Closes #5166
|
I feel the command is exposing too much of the internals that may result more confusing than useful. For example this command returns the following output from the script below (don't mind if it makes sense to import here, it is to illustrate a point) I'm not sure how useful the faker.js |
|
I've renamed But I do prefer to have a way to extract more info like what is imported and this seems to be the correct command for this. We could remove the |
ankur22
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀 I tested it with a few test scripts and seems to work as expected.
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| test, err := loadLocalTestWithoutRunner(gs, cmd, args) | ||
| if err != nil { | ||
| var unsatisfiedErr binaryIsNotSatisfyingDependenciesError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the expected behaviour when the error is a binaryIsNotSatisfyingDependenciesError? Looks like we're just ignoring it. Maybe we should log a debug log if we can ignore it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the idea is that if this is the error - this means to run this we need a new binary with som extesnions.
But as we only care about the dependancies and that is already what we've got, here we just check that if this isn't the error, as in that case we need to return it.
pablochacin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I would prefer Dependencies instead of BuildDependencies, as mentioned in another comment. But this is not blocking.
Co-authored-by: Ankur <[email protected]>
|
I am not aganst it being |
inancgumus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I think it should be dependencies instead of dependancies.
|
@mstoykov the CI is failing with errors relavant for this pull request |
What?
Add a
depscommand.This does include dependancies that might be needed for building a new binary with auto extension resolution, but also all imports.
Similar to auto extension resolution, this does take into account only imports not
requirecalls.Why?
This is useful to figure out if a given test will need to build custom binaries as well as look into dependancies in general.
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)
Closes #5166