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

Cache PNPM store in CI #23735

Merged
merged 1 commit into from
Oct 30, 2023
Merged

Cache PNPM store in CI #23735

merged 1 commit into from
Oct 30, 2023

Conversation

jonkoops
Copy link
Contributor

@jonkoops jonkoops commented Oct 5, 2023

Caches the PNPM package store on CI. This should speed up the build process and reduce the amount of request outside of GitHub infrastructure, which should also reduce network limiting on our runners.

This PR also includes the following changes:

  • All projects now use PNPM as a package manager instead of NPM, reducing complexity and improving consistency.
  • All projects now use the same Node.js installation in from the root directory, reducing the amount of total downloads.
  • All projects now automatically install Node.js and PNPM so they can be built individually (this is from the same cache, so this should have little to no performance impact).
  • The --prefer-offline flag has been added to pnpm install to reduce the amount of network requests.
  • The Node.js, PNPM and frontend-maven-plugin dependencies have been upgraded to their respective latest versions.

Closes #23695

Edit: This PR originally cached the installation for the Node.js and PNPM binaries, however these are already cached in the .m2 directory and copied over when frontend-maven-plugin is activated. I've since changed the implementation to only cache packages held in the PNPM store to reduce installation time.

@jonkoops jonkoops requested a review from a team as a code owner October 5, 2023 11:25
@jonkoops jonkoops self-assigned this Oct 5, 2023
@jonkoops jonkoops requested a review from a team as a code owner October 5, 2023 11:25
@ghost ghost added team/ui labels Oct 5, 2023
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file was referencing NPM, but it seems that it was no longer used anywhere and the versions are incredibly outdated. This seems to be covered by other logic in the GitHub Actions, so I've chosen to remove it.

prod-arguments.json Outdated Show resolved Hide resolved
@cypress
Copy link

cypress bot commented Oct 5, 2023

3 flaky tests on run #9608 ↗︎

0 538 48 0 Flakiness 3

Details:

Merge 64c3508 into ea398c2...
Project: Keycloak Admin UI Commit: 92b82e9ced ℹ️
Status: Passed Duration: 11:44 💡
Started: Oct 27, 2023 5:12 PM Ended: Oct 27, 2023 5:24 PM
Flakiness  clients_test.spec.ts • 2 flaky tests • chrome

View Output Video

Test Artifacts
Clients test > Keys tab test > Generate new keys Test Replay Output Screenshots
Clients test > Accessibility tests for clients > Check a11y violations on load/ clients list tab Test Replay Output Screenshots
Flakiness  realm_settings_general_tab_test.spec.ts • 1 flaky test • chrome

View Output Video

Test Artifacts
Realm settings general tab tests > Test all general tab switches Test Replay Output Screenshots

Review all test suite changes for PR #23735 ↗︎

@ASzc
Copy link
Contributor

ASzc commented Oct 5, 2023

I'm going to need to look at this before it is merged. Every time there's a NPM/PNPM change, it breaks the product build. Potentially this could be easier for that, as essentially we're forcing the system NPM to be used, rather than whatever frontend installs

@ASzc
Copy link
Contributor

ASzc commented Oct 5, 2023

Seems ok:

  • --prefer-offline won't break things if it works as documented. A product build will never have a local cache, but this option doesn't prevent looking online
  • None of the other modifications look like they will prevent us from using -Dskip.installnodenpm or -Dskip.installnodepnpm
  • Unifying the installs to one in the git repository root, that should work fine, but we'll need to update the product build script to match so it knows to simulate an install root there

Copy link
Contributor

@ahus1 ahus1 left a comment

Choose a reason for hiding this comment

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

Thank you for taking on this task. The changes look good, I only have to concerns about the cache key and which part of the build should be "allowed" to upload the cache. Thanks!

.github/actions/frontend-plugin-cache/action.yml Outdated Show resolved Hide resolved
@jonkoops
Copy link
Contributor Author

This PR originally cached the installation for the Node.js and PNPM binaries, however these are already cached in the .m2 directory and copied over when frontend-maven-plugin is activated. I've since changed the implementation to only cache packages held in the PNPM store to reduce installation time.

Copy link
Contributor

@ASzc ASzc left a comment

Choose a reason for hiding this comment

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

Still LGTM from maven+product perspective

ahus1
ahus1 previously approved these changes Oct 18, 2023
Copy link
Contributor

@ahus1 ahus1 left a comment

Choose a reason for hiding this comment

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

Thank you for the change. I reviewed the code and it looks good. I re-triggered some failed builds which apparently failed to download node, assuming this was a network glitch.

@jonkoops jonkoops changed the title Cache Node.js installation and PNPM store Cache PNPM store in CI Oct 18, 2023
@jonkoops
Copy link
Contributor Author

Before we go ahead and merge this, I am considering that #23695 will be closed. And although I am convinced the changes in this PR might reduce the number of instances we see of this issue, I am not entirely convinced that it will resolve it completely, as the underlying bug might exist still.

Perhaps we should donate some effort to eirslett/frontend-maven-plugin#882 to get this resolved permanently.

@ASzc
Copy link
Contributor

ASzc commented Oct 20, 2023

Getting the plugin's bug fixed seems like it will take months, and the only way I see to fix #23695 without fixing the plugin would be to implement our own download code for node/npm (at least within the CI workflows), and have frontend just use it. Kinda like the product build supplies its own.

@jonkoops
Copy link
Contributor Author

Yeah, I don't see that bug being fixed any time soon. We might consider forking the frontend-maven-plugin and adding our fixes to it while we try to upstream things.

@jonkoops
Copy link
Contributor Author

The failures on CI are due to a bug in frontend-maven-plugin that needs to be fixed (eirslett/frontend-maven-plugin#1115). I've submitted a PR (eirslett/frontend-maven-plugin#1116) to fix this, but if it stays out too long I will see if we can apply a workaround for now.

@jonkoops
Copy link
Contributor Author

jonkoops commented Oct 27, 2023

The PNPM not working on Windows situation has been resolved now that a new version of frontend-maven-plugin has been released which includes my fix. The problems with Node.js installations becoming corrupt should now also be resolved in this new version (see eirslett/frontend-maven-plugin#1118).

I am fairly confident the above will resolve #23695, so I think we're in a good spot now to get this merged. Can I ask you for another review @ASzc @ahus1?

Copy link
Contributor

@ahus1 ahus1 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for handling these two upstream issues.

@ahus1
Copy link
Contributor

ahus1 commented Oct 30, 2023

@jonkoops - LGTM, pending an UI team approval.

@ahus1 ahus1 enabled auto-merge (squash) October 30, 2023 11:17
@ahus1 ahus1 merged commit 5464205 into keycloak:main Oct 30, 2023
77 checks passed
@jonkoops jonkoops deleted the cache-node-ci branch October 30, 2023 11:51
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.

[CI] Node.exe not recognized as valid Win32 application (Java Distribution IT, Quarkus IT)
4 participants