RFC: Don't duplicate npm packages in Katello and plugins for React testing

Hello all, this is a topic that has come up a few times in the past, I believe I have a solution but I would like to get feedback from others

What we currently do

In Katello, we duplicate many packages from Foreman and the foreman-js meta packages. This is because we only npm install in Katello in our CI testing environment, and we don’t install packages from Foreman. The same may be true of other plugins.

The problem

This is an issue for two reasons:

  • In production and a development environment, the runtime dependencies come from Foreman, so what we are using for testing could be a different version than what is used in production. We are specifying the same dependency twice and can be hard to keep the versions in-sync
  • More time spent installing packages locally for developers since many are duplicated in Katello’s node_modules folder
  • In general we would like to Katello to function as a plugin and not an independent application

Proposed solution

We can use packages from Foreman with some changes to our jest configuration. We already use code from Foreman in the React tests with the changed resulting from the linked discussion. So it is easy to point jest (the React test runner) at Foreman’s node_modules.

So in CI this looks like:

  • checkout both Foreman and Katello
  • npm i in Foreman
  • Run tests in Katello

This will allow us to remove the duplicated packages from Katello.

I have a POC that can be done on a plain centos7 box, which is how I tested this method out. It uses this commit which updates the jest config to point to foreman’s node modules.

The changes that would have to happen:

  • Modify the jenkins job to checkout katello and foreman as siblings in a directory
    • side note on this: Is there any reason we checkout Foreman inside of Katello in CI today?
  • npm i only in Foreman (at least for the React portion of things) in the jenkins job
  • Make this update to Katello (will clean it up, it’s just a POC now) that updates the testing config and remove the duplicate packages.

I think this will be another step towards having our plugin ecosystem working well with the modern JS code we are using. It’s a pretty small change that we can implement to test Katello the same way we use it in a production environment and not duplicate packages.

3 Likes

I mean 3! I swear I can count :see_no_evil:

1 Like

Sounds like a good direction for me. a couple of questions:

  1. Should we add a method that trigger all plugin tests from foreman core? Right now ruby tests for plugins can be triggered from core using e.g. rake test:foreman_ansible, and when plugins are installed rake test also triggers the plugin tests.
  2. How will this work with JS CI that is mostly currently done on travis? Would those jobs need modification or migration into jenkins?

I’m open to it

For Katello, we just run linting and I haven’t figured out how to not duplicate the linting packages so this shouldn’t be affected, the linting packages will stay for now and travis will keep working. Though I think the linting could probably just be done in Jenkins for Katello. I think in the future we can convert to foreman’s eslint plugin to make things easier and possibly remove the duplicate eslint packages.

For other plugins that do the React testing in Travis, the change should be able to be accommodated. I was actually testing this out in github actions and was making progress. If you can check out both the directories, npm install, and npm test, all should work as expected.

Side note: gh actions will likely not work for Katello because we have to npm install the packages in Jenkins to test assets and we also run the tests for Satellite which does not use github.

I also think the “find where foreman is checked out” logic that is part of Katello’s jest.config.js can be moved to it’s own npm package so other plugins can use it.

I would like to clarify: the proposed change will not break any plugins, no changes will have to be made to Foreman core for this to work, its logic on the plugin side to find foreman and use it’s node modules.

ESLint 7 mentioned in their changelog that paths will become relative to the working directory rather than the entry directory:

Would that solve it?

Thanks for sharing, perhaps it would, I would have to poke around more. I imagine we want to use https://www.npmjs.com/package/@theforeman/eslint-plugin-foreman in katello and standardize everything the same as Foreman best we can, but I still have to figure out how that works across repos.

Overall, I think it would be best to tackle the linting separately from the changes proposed here, linting packages are not runtime packages so it’s not as big of a concern as duplicating runtime packages. The changes proposed here may help make things easier, I definitely would like to not duplicate the linting packages and standardize more with Foreman in the not-too-distant future.

Thanks for bringing that up @John_Mitsch, that approach make sense.

Another way that might work is by installing foreman in katello as an npm module.
I recall myself experiencing with that a little bit locally by running npm install ../foreman in katello and configuring jest/storybook/lint to use foreman from node_modules.

This would keep the package versions in-sync, but would still duplicate all the packages from foreman right? Because they would be installed in foreman and in katello’s node_modules.

Well, if you would have "foreman": "../foreman", in your package.json and you would run npm i. Then, npm should only create a symlink.

If you don’t have foreman cloned locally, you might be able to get it (source and deps) from github with something like this in your package.json "foreman": "https://github.com/theforeman/foreman",.

Interesting, it does create the symlink as you said. The only thing to consider is always having foreman in the same location. The advantage of handling things in jest is that you can do more logic in the config file since it is in JavaScript vs. the package.json file which is JSON. Currently we search for foreman in these locations because in CI we checkout Foreman inside of Katello (I never did find out why we do that).

Though part of this RFC is to check out the Foreman and Katello repos as siblings, same as the development environment, so making that a standard across dev and CI environments is worth considering and we can used the fixed path.

I opened a POC here https://github.com/Katello/katello/pull/8722

I was able to use the Foreman dependencies and I used github actions for the react tests, which I am proposing we move the React tests to for Katello, assuming they work as expected.

I suspect it’s because the default way to check out a git repo in Jenkins is directly in the workspace. AFAIK there’s no need for this and can be changed.