Skip to content
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

headlamp-plugin: Make the package arg of the build command optional #321

Merged
merged 5 commits into from
Sep 23, 2021

Conversation

joaquimrocha
Copy link
Collaborator

We rely on just calling the "headlamp-plugin build" command without
any arguments, but the build command expected a positional argument as
required, so the default and advised case for building plugins was
failing.

We rely on just calling the "headlamp-plugin build" command without
any arguments, but the build command expected a positional argument as
required, so the default and advised case for building plugins was
failing.
@illume illume force-pushed the do_not_require_a_positional_arg_in_headlamp_plugin branch 7 times, most recently from a7ae5d6 to f299615 Compare September 21, 2021 06:24
There's an issue with npm v7 looping on dependencies.
@illume illume force-pushed the do_not_require_a_positional_arg_in_headlamp_plugin branch from f299615 to 6cd5a73 Compare September 21, 2021 07:19
@illume
Copy link
Collaborator

illume commented Sep 21, 2021

interestingly, the CI found a bug when using npm 7. Where npm 7 gets into a loop resolving dependencies, and keeps processing until it runs out of memory. See npm/cli#3359 and npm/cli#3208

There's a work around for running it on CI with npm 7, but users of npm 7 will encounter this bug.

I tried updating the plugin dependencies, and still get the issue. There's another work around or two to try(which I will do next)... but ultimately the bug in npm 7 needs fixing (or maybe it's a bug in one of the dependencies).

@illume illume marked this pull request as draft September 21, 2021 07:31
Additionally we stop using --link inside test-headlamp-plugin.sh
so that it tests more closely what users will run. Previously this
would hang when using npm v7.
@illume illume force-pushed the do_not_require_a_positional_arg_in_headlamp_plugin branch from 0281ead to 37ac355 Compare September 21, 2021 07:51
@illume
Copy link
Collaborator

illume commented Sep 21, 2021

Adding a package-lock.json generated with npm v6 to the template lets npm v7 work.

@illume illume marked this pull request as ready for review September 21, 2021 08:00
Copy link
Collaborator

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 🎉

@illume
Copy link
Collaborator

illume commented Sep 21, 2021

Probably we should release a new @kinvolk/headlamp-plugin version.

Copy link
Collaborator Author

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@illume , It all LGTM. The only thing I noticed is that we're not testing the headlamp-plugin build (no argument) which is what failed initially, or maybe I missed it?

@illume
Copy link
Collaborator

illume commented Sep 21, 2021

@joaquimrocha yeah. It's not directly calling headlamp-plugin build... but it does so through npm run build which calls that. I'll update the test to call headlamp-plugin build with no argument directly.

- print bash commands for easier debugging
- improve build test, so it runs headlamp-plugin build
@illume
Copy link
Collaborator

illume commented Sep 23, 2021

The headlamp-plugin build is tested better now, and also added a test for headlamp-plugin extract.

Copy link
Collaborator Author

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@joaquimrocha joaquimrocha merged commit 1bd8707 into main Sep 23, 2021
@joaquimrocha joaquimrocha deleted the do_not_require_a_positional_arg_in_headlamp_plugin branch September 23, 2021 11:42
@illume illume added the plugins label Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants