search

theforeman / foreman_rh_cloud

a plugin to Foreman that generates and uploads reports to the Red Hat cloud
GNU General Public License v3.0
6 stars 32 forks source link

Fix tests on pnpm install #831

Closed chris1984 closed 1 year ago

chris1984 commented 1 year ago

@ShimShtein I got this to work on my local container, but something is blocking the pined version I set, it looks like it's using the locks in the config directory? I tried to update those but git does not see that change when I push it up. I can see in my local package-lock.json that 4.3 is loaded in

ShimShtein commented 1 year ago

The build process fails because it still uses the foreman_3_7 branch of the code, so the package.json file is not taken into account. We need to improve the build process to refresh the branch from the latest commit. Something similar to the command that happens here: https://github.com/theforeman/foreman_rh_cloud/actions/runs/5789544894/job/15690805100?pr=831#step:3:49

I think there should be a way to pass parameters into the dockerfile. Worst comes to worse, we can modify the dockerfile itself in the build_test_container action. Apparently it's an option: https://docs.docker.com/build/guide/build-args/

chris1984 commented 1 year ago

@ShimShtein updated you were right we don't need to pin the pnpm version

From my local container:

sh-4.4# pnpm -v
7.33.6
sh-4.4# pwd
/projects/foreman
sh-4.4# pnpm install
Lockfile is up to date, resolution step is skipped
Already up to date

   ╭──────────────────────────────────────────────────────────────────╮
   │                                                                  │
   │                Update available! 7.33.6 → 8.6.12.                │
   │   Changelog: https://github.com/pnpm/pnpm/releases/tag/v8.6.12   │
   │                Run "pnpm add -g pnpm" to update.                 │
   │                                                                  │
   │      Follow @pnpmjs for updates: https://twitter.com/pnpmjs      │
   │                                                                  │
   ╰──────────────────────────────────────────────────────────────────╯

> TheForemanDevDeps@3.6.0 postinstall /projects/foreman
> ./script/npm_install_plugins.js

Lockfile is up to date, resolution step is skipped
Lockfile is up to date, resolution step is skipped
Lockfile is up to date, resolution step is skipped
Lockfile is up to date, resolution step is skipped
Lockfile is up to date, resolution step is skipped
Already up to date
Already up to date
Already up to date
Already up to date
Already up to date

optionalDependencies: skipped

Done in 1.3s

optionalDependencies: skipped

Done in 1.6s

optionalDependencies: skipped

optionalDependencies: skipped

Done in 1.9s

Done in 1.9s

optionalDependencies: skipped

Done in 2s
Done in 5.2s

sh-4.4# npm ls | grep cosmic
| | | | | +-- cosmiconfig-typescript-loader@4.3.0 -> /projects/foreman/node_modules/.pnpm/cosmiconfig-typescript-loader@4.3.0/node_modules/cosmiconfig-typescript-loader deduped
| |   +-- cosmiconfig@6.0.0
+-- cosmiconfig-typescript-loader@4.3.0 -> /projects/foreman/node_modules/.pnpm/cosmiconfig-typescript-loader@4.3.0/node_modules/cosmiconfig-typescript-loader
| +-- cosmiconfig@5.2.1
| +-- cosmiconfig@5.2.1
ShimShtein commented 1 year ago

Merged, thanks @chris1984 !