SAK-52129 webcomponents Add skip entry to npm test configuration #14299
Conversation
WalkthroughPropagates Maven's test-skip flag to the frontend-maven-plugin, updates Docker build to use a different Maven base image and fetch Sakai via git during build, removes a Docker Maven settings file, makes tomcat entrypoint append a secrets file only if it exists, and updates Docker README build instructions. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webcomponents/tool/pom.xml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
**/*.{html,jsp,vm,xhtml,xml}: Use Bootstrap 5.2 as the preferred UI framework for styling in Sakai frontends
Leverage Bootstrap 5 components for consistent UI/UX
Files:
webcomponents/tool/pom.xml
**/*.{html,jsp,vm,xhtml,xml,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Ensure all UI components work across different screen sizes (Responsive Design)
Files:
webcomponents/tool/pom.xml
**/*.{js,html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Use the web components in the webcomponents/ directory when possible in Sakai frontends
Files:
webcomponents/tool/pom.xml
**/*.{html,jsp,jspx,xml,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer kebab-case for values of HTML class and id attributes
Files:
webcomponents/tool/pom.xml
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to webcomponents/**/src/**/*.{ts,tsx,js} : Define custom HTML elements for Sakai-specific functionality using web components
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to webcomponents/tool/src/main/frontend/**/*.js : Build new Sakai frontend features as Web Components using the Lit library
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to webcomponents/tool/src/main/frontend/**/*.js : Define custom elements for Sakai-specific functionality when building Web Components
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2025-11-24T19:11:31.821Z
Learning: Applies to **/*.{js,html,jsp,vm,xhtml,xml} : Use the web components in the webcomponents/ directory when possible in Sakai frontends
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Use Maven commands (install, clean install sakai:deploy, test) and frontend commands (lint, bundle, analyze) as given
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to webcomponents/tool/src/main/frontend/**/*.js : Define custom elements for Sakai-specific functionality when building Web Components
Applied to files:
webcomponents/tool/pom.xml
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to webcomponents/tool/src/main/frontend/**/*.js : Build new Sakai frontend features as Web Components using the Lit library
Applied to files:
webcomponents/tool/pom.xml
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Use Maven commands (install, clean install sakai:deploy, test) and frontend commands (lint, bundle, analyze) as given
Applied to files:
webcomponents/tool/pom.xml
🪛 GitHub Actions: Maven Build
webcomponents/tool/pom.xml
[error] 132-132: Maven build failed: Non-parseable POM due to 'Duplicated tag: phase'. See log for details.
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: sakai-deploy
🔇 Additional comments (1)
webcomponents/tool/pom.xml (1)
134-134: Good addition: honors Maven test-skipping behavior.The skip configuration correctly uses the standard
${maven.test.skip}property, allowing users to skip frontend tests viamvn install -Dmaven.test.skip=true. This aligns with Maven conventions and the PR objectives.
adrianfish
force-pushed
the
SAK-52129
branch
from
bd54193 to
c7233ca
Compare
|
So I checked out your branch and it indeed fixed the issue - thanks! I suggest two more changes. This makes it a lot more flexible for dev testing. I don't know how to push these changes through github so I will just paste a patch here. Old School. |
adrianfish
force-pushed
the
SAK-52129
branch
from
c7233ca to
a4b8609
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docker/tomcat/bin/entrypoint.sh (1)
3-6: Good defensive programming – secrets file guard prevents errors.The conditional check ensures the script doesn't fail when the secrets file is absent. This is appropriate for environments where Docker secrets may not be configured.
Optional: quote the variable in the test condition for robustness
While unlikely to cause issues here, quoting variables in test conditions is a shell scripting best practice:
-if [ -f $PROPS ]; then +if [ -f "$PROPS" ]; thenThis prevents unexpected behavior if the variable ever contains spaces or special characters.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docker/Dockerfile.source(1 hunks)docker/README.md(1 hunks)docker/maven/settings.xml(1 hunks)docker/tomcat/bin/entrypoint.sh(1 hunks)webcomponents/tool/pom.xml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docker/maven/settings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- webcomponents/tool/pom.xml
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Use Maven commands (install, clean install sakai:deploy, test) and frontend commands (lint, bundle, analyze) as given
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to webcomponents/tool/src/main/frontend/**/*.js : Build new Sakai frontend features as Web Components using the Lit library
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to webcomponents/tool/src/main/frontend/**/*.js : Define custom elements for Sakai-specific functionality when building Web Components
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Deploy to Tomcat using `mvn clean install sakai:deploy -Dmaven.tomcat.home=/path/to/tomcat`
Applied to files:
docker/Dockerfile.source
📚 Learning: 2025-11-24T19:12:20.537Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Use `mvn clean install sakai:deploy -Dmaven.tomcat.home=/path/to/tomcat` to deploy to Tomcat
Applied to files:
docker/Dockerfile.source
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Use Maven commands (install, clean install sakai:deploy, test) and frontend commands (lint, bundle, analyze) as given
Applied to files:
docker/Dockerfile.source
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Target Java 17 for trunk (Java 11 for Sakai 22/23 historical context)
Applied to files:
docker/Dockerfile.source
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to **/*.java : Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Applied to files:
docker/Dockerfile.source
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (3)
docker/Dockerfile.source (2)
4-4: Good parameterization for flexible repository selection.The new
repositoryARG with a sensible default allows developers to build from alternative repositories (e.g., forks or feature branches) without modifying the Dockerfile.
10-12: Clone with branch flag correctly eliminates the separate checkout step.Using
-b ${release}in the git clone command directly checks out the desired branch, making the previous explicitgit checkout ${release}step unnecessary. The shallow clone (--depth=1) keeps the image build efficient.docker/README.md (1)
39-41: Clear documentation of the repository parameterization feature.The example demonstrates how to build from an alternative repository and branch, making the new build-arg functionality discoverable for developers who need to test custom branches or forks.
adrianfish
force-pushed
the
SAK-52129
branch
from
a4b8609 to
15ffb6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docker/Dockerfile(1 hunks)docker/README.md(1 hunks)docker/maven/settings.xml(0 hunks)docker/tomcat/bin/entrypoint.sh(1 hunks)webcomponents/tool/pom.xml(1 hunks)
💤 Files with no reviewable changes (1)
- docker/maven/settings.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- webcomponents/tool/pom.xml
- docker/tomcat/bin/entrypoint.sh
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Deploy to Tomcat using `mvn clean install sakai:deploy -Dmaven.tomcat.home=/path/to/tomcat`
Applied to files:
docker/Dockerfile
📚 Learning: 2025-11-24T19:12:20.537Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Use `mvn clean install sakai:deploy -Dmaven.tomcat.home=/path/to/tomcat` to deploy to Tomcat
Applied to files:
docker/Dockerfile
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Use Maven commands (install, clean install sakai:deploy, test) and frontend commands (lint, bundle, analyze) as given
Applied to files:
docker/Dockerfile
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (2)
docker/Dockerfile (2)
10-12: LGTM!The Maven configuration looks good. Using
-T 1Cfor parallel builds,--no-transfer-progressto reduce log noise, and-Dmaven.test.skip=truealigns with the PR objective for test skipping. Thesakai:deploy-explodedgoal with the Tomcat home path is consistent with project learnings.
14-31: LGTM!The runtime stage properly copies configuration and build artifacts from the build stage to the Tomcat image. The entrypoint setup is correct.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker/README.md (1)
33-33: Fix build argument names and filename.This line has multiple issues:
- Uses
--build-arg release=masterbut Dockerfile definesARG branch=master(line 6)- References
./Dockerfile.sourcebut the actual file is./Dockerfile🔎 Apply this diff to fix the command:
- 1. Execute `docker build --build-arg release=master -t sakai -f ./Dockerfile.source .` substituting "master" for the branch/tag you wish to build + 1. Execute `docker build --build-arg branch=master -t sakai -f ./Dockerfile .` substituting "master" for the branch/tag you wish to build
🧹 Nitpick comments (1)
docker/Dockerfile (1)
8-8: Remove redundant git checkout.The
-b ${branch}flag ingit clonealready checks out the specified branch, making the subsequentgit checkout ${branch}redundant.🔎 Apply this diff to remove the redundant operation:
-RUN apk add git && git clone -b ${branch} --depth=1 ${repo} . && git checkout ${branch} +RUN apk add git && git clone -b ${branch} --depth=1 ${repo} .
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docker/Dockerfile(1 hunks)docker/README.md(1 hunks)docker/maven/settings.xml(0 hunks)docker/tomcat/bin/entrypoint.sh(1 hunks)webcomponents/tool/pom.xml(1 hunks)
💤 Files with no reviewable changes (1)
- docker/maven/settings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- webcomponents/tool/pom.xml
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Deploy to Tomcat using `mvn clean install sakai:deploy -Dmaven.tomcat.home=/path/to/tomcat`
Applied to files:
docker/Dockerfile
📚 Learning: 2025-11-24T19:12:20.537Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Use `mvn clean install sakai:deploy -Dmaven.tomcat.home=/path/to/tomcat` to deploy to Tomcat
Applied to files:
docker/Dockerfile
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to **/*.java : Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Applied to files:
docker/Dockerfile
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Use Maven commands (install, clean install sakai:deploy, test) and frontend commands (lint, bundle, analyze) as given
Applied to files:
docker/Dockerfile
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (4)
docker/tomcat/bin/entrypoint.sh (1)
3-6: LGTM! Good defensive programming.The conditional check prevents errors when the security properties file is not mounted as a secret, making the configuration optional rather than required.
docker/Dockerfile (3)
5-6: Good use of build arguments for flexibility.The ARG declarations enable building from different repositories and branches as documented in the README, while providing sensible defaults.
12-12: Build command flags are appropriate.The consolidated Maven build uses good practices: parallel builds (
-T 1C), progress suppression (--no-transfer-progress), and test skipping (-Dmaven.test.skip=true) for faster Docker image creation.
1-1: Remove redundant git checkout command on line 8.The
git clone -b ${branch}command already checks out the specified branch, making the subsequentgit checkout ${branch}redundant.Regarding Chrome: The base image change is safe. While webcomponents tests use headless browser testing (web-test-runner), the Docker build explicitly skips all Maven tests via
-Dmaven.test.skip=trueon line 12, so Chrome is not required.Likely an incorrect or invalid review comment.
2 participants
https://sakaiproject.atlassian.net/browse/SAK-52129
Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.