Update to fix yaml error in pom and README.md content - #65
Conversation
…ailed project overview and usage instructions
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughBumps project version to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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: 2
🤖 Fix all issues with AI agents
In `@src/main/resources/static/README.md`:
- Line 30: The README's Java version statement ("Java 21+") is inconsistent with
the project's compiler target; update either the README or the Maven build
configuration so they match: change the README line to reflect the actual
maven.compiler.release value (currently 25) or adjust the
<maven.compiler.release> in pom.xml to 21 (or your desired minimum) so README
and the Maven property maven.compiler.release are aligned; ensure the README
text and the pom.xml property remain consistent going forward.
- Line 166: The README note saying "Running the packaged JAR directly with `java
-jar` is not supported..." is now incorrect because this PR adds the
maven-shade-plugin to produce a fat JAR; update README.md to either remove that
sentence or replace it with a short note stating that a shaded (fat) JAR is
produced and can be run with `java -jar target/<artifact>-<version>-shaded.jar`,
and optionally mention the maven-shade-plugin or "shaded JAR" so readers know
dependencies are bundled.
🧹 Nitpick comments (1)
src/main/resources/static/README.md (1)
39-55: Add language identifiers to fenced code blocks.Several fenced code blocks (e.g., lines 39, 141, 156, 162, 174, 269, 322, 364, 370, 386) lack a language specifier. Adding identifiers like
text,yaml, orbashimproves rendering and satisfies markdownlint MD040.Also applies to: 141-143, 156-158, 162-164, 174-176, 269-271, 322-324, 364-366, 370-372, 386-388
There was a problem hiding this comment.
Hi, did we plan to make a new release patch already?
Our deployment is scheduled on thursdays so not sure if I have missed something? : )
I dont think we should create a patch just for a readme update, that can be saved until next thursday when we have more sufficient progress.
However I can give you my review:
1: We did not need to update the POM version according to Martin, I think I forgot to revert it back to 1.0-SNAPSHOT as it was from the beginning, so maybe we can do that here.
2: This release does not run, and the docker image throws an exception:
➜ ~ docker run -p 8080:8080 ghcr.io/ithsjava25/project-webserver-juv25d:v1.0.1-beta
Exception in thread "main" java.lang.NoClassDefFoundError: org/yaml/snakeyaml/Yaml
at org.juv25d.util.ConfigLoader.loadConfiguration(ConfigLoader.java:26)
at org.juv25d.util.ConfigLoader.<init>(ConfigLoader.java:15)
at org.juv25d.util.ConfigLoader.getInstance(ConfigLoader.java:20)
at org.juv25d.App.main(App.java:15)
Caused by: java.lang.ClassNotFoundException: org.yaml.snakeyaml.Yaml
at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(Unknown Source)
at java.base/java.lang.ClassLoader.loadClass(Unknown Source)
... 4 more
➜ ~ So Im not sure what the issue was with the JAR to begin with for these changes to be made?
Yesterday tested the initial release v1.0.0-beta both the raw JAR and the docker image and they ran fine before finishing up the deployment.
I had also already updated the maven-jar-plugin to point to the main class and ./mvnw package built a successful manifest and JAR package that could run.
Could you please elaborate on what errors you were facing to make these changes?
Lastly: If we are to keep this release patch, the version tag should not be v1.0.2-beta.
This is still a beta so imo the correct patch version would be v1.0.0-beta.1 or something similar to that.
Until we release a full working version next Thursday then we move back to v1.0.0 -> v1.0.1 etc.
I had not realised we had planned for just weekly updates (since the project is only for two more weeks, that didnt even occur to me to be a thing). The patch however is not just for the README.md file, but I do agree that it is a minor patch. According to the error I got when trying to run the .jar file (both locally and through docker, same as you did): Exception in thread "main" java.lang.NoClassDefFoundError: org/yaml/snakeyaml/Yaml
at org.juv25d.util.ConfigLoader.loadConfiguration(ConfigLoader.java:26)
at org.juv25d.util.ConfigLoader.<init>
(ConfigLoader.java:15)
at org.juv25d.util.ConfigLoader.getInstance(ConfigLoader.java:20)
at org.juv25d.App.main(App.java:15)
Caused by: java.lang.ClassNotFoundException: org.yaml.snakeyaml.Yaml
at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(Unknown Source)
at java.base/java.lang.ClassLoader.loadClass(Unknown Source)
... 4 more
there was a missing component in the pom.xml that caused us to not build a fat JAR, meaning the SnakeYAML library after install for the execution. I believe this SnakeYAML dependency was not present for the previous release. I'm not sure when the snakeyaml was added to the pom.xml, but it seems it is connected to the configloader. After testing now, I seem to be able to run the app without the dependency in pom.xml, meaning we shouldn't even need it for the .jar to run...? If we had decided on a weekly release on Thursdays, we should revert back to the previous release and just keep working on the project in PRs as usual - apologies for the mix-up! Should I simply remove the latest release? |
|
The snakeYaml dependency was added by me in #50 to read the application-properties in ConfigLoader. And yes it was not included in the previous release. Yeah locally your version is running, I also downloaded the JAR from the release and that ran fine, but I think there may have been an issue with how you deployed? Im guessing you created a tag and release manually through github GUI? Cause one thing that stands out to me here is that the docker build ran before your commits was made in this branch. The docker build workflow ran on Today at 2:14 PM, the release was then created at Today at 2:46 PM, but your first commit / push to this branch was Today at 2:47 PM. Maybe thats why the JAR and branch work locally, but the docker image was triggered before which means its not in sync with this branch. Cause the docker workflow is configured to trigger the workflow as soon as we create a new tag. |
|
No dont worry, for me it is fine if you want to push a patch release and do continues delivery. However maybe we still need to delete and revert the current release you did and deploy again but fixing the docker image. My suggestion is to not create a tag in the gh GUI from the main branch. That way we can also test everything in this PR before merging ! |
The version before this fix does not work locally as a .jar, though, since the library required does not get packed with it without this fix to the pom.xml. I merged the commit, pushed the tag from the terminal, and then I had to go in on github to create the release. I had an error under setup QEMU (which apparently is a common type of error due to some networking error on github /docker hub side), so I had to rerun the task to get it through - maybe this has something to do with the weird timings of the workflow commits? So the proper process would be to push a branch with a tag and that would get released as soon as it gets merged? I've never done these types of tasks before, so for me it's all learning-as-I-go (which is still valuable, but quite frustrating). |
|
Got you, not sure what you mean when you say merged the commit, cause the PR is not merged, so Im taking a guess you meant to say pushed the commit? And I think then you pushed the tag to main branch right? otherwise we would see the workflow run here which it is not. Try deleting the tag and the release from github, then re-create and push the tag directly to this branch, I think that would solve the issue. |
|
if u need any support just ping me and we can get on a call and do it together |
I created the tag on main after I hade merged the latest two commits. After I got the release out and found out there was an error, I created this branch, commited and pushed and created the PR. |
|
I pushed a tag here now, so we can test the docker image and understand whats going on exactly |
|
I think I see whats going wrong, the new docker image is not working, which means you did it correctly, but mvn is producing 2 different jars when I run ./mvnw package locally. I will push a fix |
… rollback pom version
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/resources/static/README.md`:
- Line 166: The README's JAR filename is incorrect: it refers to
target/JavaHttpServer-1.0.2-beta.jar but the Maven Shade Plugin sets
<finalName>app</finalName>, so update the README sentence to show the actual
artifact produced (e.g., run with `java -jar target/app.jar`) or else change the
shade plugin's <finalName> to match the README; locate the README line
containing the example command and replace the jar name, or adjust the pom.xml
<finalName> to the README's name so they match.
🧹 Nitpick comments (2)
src/main/resources/static/README.md (1)
39-55: Add language specifiers to fenced code blocks.Several fenced code blocks lack a language identifier (e.g., the request lifecycle diagram, URL blocks, directory paths). Adding a language hint (e.g.,
text,yaml,plaintext) satisfies markdownlint MD040 and improves rendering in some Markdown viewers.Also applies to: 141-143, 156-158, 162-164, 174-176, 269-271, 322-323, 364-366, 370-372, 386-388
pom.xml (1)
161-183: Maven Shade Plugin configuration is correct for producing a fat JAR.The setup properly bundles dependencies (notably SnakeYAML) into a single JAR, resolving the
NoClassDefFoundErrorreferenced in the PR. ThefinalName=appensures a predictable artifact name for the Docker build.Version 3.5.1 (2023) has a newer release available: 3.6.1 (September 2025). Consider updating to the latest version for any recent bug fixes or improvements, though the current version is fully functional.
|
it works now, so you can delete the release and tag v1.0.1-beta and create a new release from v1.0.0-beta.2 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 141-143: The fenced code block containing the URL (the
triple-backtick block around "http://localhost:8080") needs a language
specifier; update that block to include a language such as "text" (i.e., change
``` to ```text) so the fenced code block is Markdown-compliant and renders with
explicit language.
- Around line 162-164: The fenced code block containing the URL
"http://localhost:8080" in README.md lacks a language specifier; edit that block
to add a language (e.g., use ```text) before the URL and keep the closing ``` so
the block is labeled and renders correctly.
- Line 166: Update the README note that references the fat JAR filename: replace
the incorrect `JavaHttpServer-<version>.jar` with the actual artifact name
`app.jar` (the Maven Shade Plugin sets `<finalName>app</finalName>`), so the
sentence reads that you can run the project with `java -jar target/app.jar`.
Gonna need you to explain this, but now it's weekend o/ |
3 participants
Add Maven Shade Plugin for building a fat JAR; update README with detailed project overview and usage instructions
Summary by CodeRabbit
Chores
Documentation