Skip to content

Install NPM packages & compile webpack#18

Merged
ekohl merged 7 commits intov0from
install-npm
Jan 9, 2024
Merged

Install NPM packages & compile webpack#18
ekohl merged 7 commits intov0from
install-npm

Conversation

@ekohl
Copy link
Member

@ekohl ekohl commented Oct 31, 2023

This only happens if package.json exists.

Opening, but needs test cases. foreman_ansible has package.json, but foreman_expire_hosts doesn't.

@ekohl
Copy link
Member Author

ekohl commented Oct 31, 2023

I'm starting to wonder how to properly install NPM for plugins, as well as how to make sure there is caching in place for it.

@evgeni evgeni force-pushed the install-npm branch 2 times, most recently from f10dbad to 9371c59 Compare November 30, 2023 13:43
@evgeni
Copy link
Member

evgeni commented Nov 30, 2023

@ekohl
Copy link
Member Author

ekohl commented Nov 30, 2023

That appears to work. Anything else we should do here? Do you want to apply the same matrix style testing of multiple jobs that we apply in core?

@evgeni
Copy link
Member

evgeni commented Nov 30, 2023

That's a good question I have no definitive answer for.

I guess it'd be a good idea to run test:$plugin, assets:precompile[$plugin] and db:seed.

For the later we have a separate job, but Leos' is right we should test both paths, all migrations in one go, and foreman first, then plugin. So maybe those three as a matrix and the current separate one kept separate?

@evgeni evgeni force-pushed the install-npm branch 3 times, most recently from ca9f637 to baa47f7 Compare December 12, 2023 08:40
Comment on lines +131 to +135
- name: "Compile webpack"
if: ${{ hashFiles(format('{0}/package.json', inputs.plugin)) != '' && contains(matrix.task, 'test') }}
run:
bundle exec rake webpack:compile
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, if test:$plugin depends on webpack (because it contains integration tests), it should depend on that itself via Rake, and not make us do this hack, but 🤷‍♀️

Copy link
Member Author

Choose a reason for hiding this comment

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

You could argue that it can make it painful for developers. If you have webpack running then you don't need to compile, but we can't detect this. So I can accept this now. Perhaps after we merge webpack 5 we can be a bit smarter and introduce some task that only compiles if some file doesn't exist.

@evgeni evgeni marked this pull request as ready for review December 12, 2023 09:19
@evgeni evgeni linked an issue Dec 12, 2023 that may be closed by this pull request
@ekohl
Copy link
Member Author

ekohl commented Jan 9, 2024

I can't approve since it's my own PR, so let's try this out by me merging it.

@ekohl ekohl merged commit 803029e into v0 Jan 9, 2024
@ekohl ekohl deleted the install-npm branch January 9, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

Bring back changes from Foreman core's workflow to foreman_plugins

2 participants