-
Notifications
You must be signed in to change notification settings - Fork 11
Add bundler ecosystem extension #1182
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
Conversation
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.
Might need some testing (which I believe @maxrake is currently doing), but the code looks good now.
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.
Local testing was performed with some limitations. The Phylum processing pipeline was delayed beyond the 30 minutes advertised and therefore it was not possible to see the full results of some phylum bundle add
commands. More local testing will be performed after the requested changes are addressed.
It would also be nice if this extension included tests, like how the poetry
extension does.
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.
Local testing was performed and the extension worked as expected for the install
, add
, and update
commands.
There is still a question about the remove
command, which is not covered but does install gems. The README
says:
When invoking
phylum bundle
, subcommands that would install gems will trigger a Phylum analysis first.
Adding the remove
command for coverage or creating a new issue for it and any other affected ecosystem extensions is requested.
It would also be nice if this extension included tests, like how the Poetry extension does.
This wouldn't be difficult, but should be made a consistent policy if we want to enforce it. Bundle also has an option to not install packages on remove for example.
The poetry extension tests have never provided anything but trouble to me. I didn't write them and I wasn't ever a fan of them. So I have no intention of adding more for this extension. |
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.
The latest changes were confirmed with local testing. LGTM.
Closes #1178.