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 Node.js artifacts from maven-frontend-plugin on CI #33815

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

jonkoops
Copy link
Contributor

Closes #31835

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 this PR. See below for some concerns I have when reviewing this change.

Comment on lines 57 to 64
- uses: actions/cache@v4
name: Cache Node.js related artifacts
with:
path: |
~/.m2/repository/com/github/eirslett/node
~/.m2/repository/com/github/eirslett/pnpm
key: ${{ runner.os }}-frontend-plugin-artifacts
Copy link
Contributor

Choose a reason for hiding this comment

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

The cache key is static. Therefore it will be cached once, and the cache is never refreshed for a different node or NPM version, which is then downloaded all the time. Consider adding a combined cache key for npm & node.

This doesn't respect the setting create-cache-if-it-doesnt-exist. This could lead to a situation where there is no frontend being built, and then an empty cache is being created. Please create the cache only when this is set, and provide a download-only version when it is not set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cache key is static. Therefore it will be cached once, and the cache is never refreshed for a different node or NPM version, which is then downloaded all the time. Consider adding a combined cache key for npm & node.

Done. The key for the cache now includes both the Node.js version and the PNPM version.

This doesn't respect the setting create-cache-if-it-doesnt-exist. This could lead to a situation where there is no frontend being built, and then an empty cache is being created.

Like we discussed offline, the directory paths are very specific and would only create an artifact if the directories exist and contain files. Hence this should not be a problem for this specific cache action.

@jonkoops jonkoops force-pushed the cache-nodejs-artifacts branch 3 times, most recently from f930886 to f72578e Compare October 16, 2024 11:37
@keycloak-github-bot

This comment was marked as off-topic.

keycloak-github-bot[bot]

This comment was marked as off-topic.

@jonkoops jonkoops force-pushed the cache-nodejs-artifacts branch 2 times, most recently from 8ea2480 to 0b0270b Compare October 16, 2024 11:52
@jonkoops jonkoops force-pushed the cache-nodejs-artifacts branch from e58db6f to cf707b5 Compare October 16, 2024 14:51
@jonkoops
Copy link
Contributor Author

jonkoops commented Oct 16, 2024

One problem that remains is that it is not possible to exclude the existing artifacts that are cached from org/github/eirslett in the Maven cache due to actions/toolkit#713. Worst case scenario we get two versions of Node.js in the cache, but the fix should still work, just with a little more overhead.

@jonkoops jonkoops requested review from ahus1, pskopek and stianst October 16, 2024 15:00
@jonkoops
Copy link
Contributor Author

@pskopek @stianst since you both have experience with the actions can I ask you for a review as well?

Copy link

@keycloak-github-bot keycloak-github-bot bot left a comment

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@keycloak-github-bot
Copy link

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled

Keycloak CI - Java Distribution IT (windows-latest - temurin - 17)

java.lang.AssertionError: Did not find the event of expected type USER_DISABLED_BY_TEMPORARY_LOCKOUT. Events present: [IDENTITY_PROVIDER_POST_LOGIN_ERROR, IDENTITY_PROVIDER_POST_LOGIN_ERROR, IDENTITY_PROVIDER_POST_LOGIN_ERROR, IDENTITY_PROVIDER_POST_LOGIN_ERROR]
	at org.junit.Assert.fail(Assert.java:89)
	at org.keycloak.testsuite.AssertEvents$ExpectedEvent.assertEvent(AssertEvents.java:410)
	at org.keycloak.testsuite.broker.AbstractAdvancedBrokerTest.testPostBrokerLoginFlowWithOTP_bruteForceEnabled(AbstractAdvancedBrokerTest.java:586)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...

Report flaky test

@stianst stianst merged commit 96b6cb4 into keycloak:main Oct 17, 2024
77 checks passed
@jonkoops jonkoops deleted the cache-nodejs-artifacts branch October 17, 2024 10:20
Petar-CV pushed a commit to Petar-CV/keycloak that referenced this pull request Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows builds fail too often due to problems with the download of Node
3 participants