Ci - add build workflow - #28
Conversation
Rajath2005
left a comment
There was a problem hiding this comment.
Thank you for your contribution! I appreciate the effort you've put into setting up a GitHub Actions CI workflow. Automating the compilation process is a great addition and will help maintain the project's quality as it grows.
After reviewing the changes, I found one issue that needs to be addressed before this PR can be merged:
Required Change
-
The build status badge added to the
README.mdcurrently points to your fork:https://github.com/Tova-Rozovsky/SmartCityApp/actions/workflows/ci.yamlSince this repository is
Rajath2005/SmartCityApp, the badge should reference the main repository instead so it reflects the actual build status of the project after merging. Please update the badge URL accordingly.
Optional Improvement
- Instead of installing Java manually using
apt-get, consider using the officialactions/setup-javaGitHub Action. It is the recommended approach by GitHub, provides better caching, and simplifies the workflow configuration.
Overall, this is a valuable contribution. Once the README badge is updated to point to the main repository, I'll be happy to review the updated changes again.
Thanks again for contributing to SmartCityApp!
|
Hi @Rajath2005, |
Rajath2005
left a comment
There was a problem hiding this comment.
Thanks for the update and for addressing the previous feedback! I appreciate the improvements to the workflow and the README badge.
I noticed one remaining issue before we can merge:
- The path filter currently uses
src/**.java. Since the project's Java files are organized in nested packages (for example,src/com/smartcity/...), this should be updated tosrc/**/*.javaso that changes to Java files in any subdirectory correctly trigger the workflow.
Additionally, as a small cleanup, the Handle Failure step can be removed because GitHub Actions automatically marks the job as failed if javac exits with a non-zero status.
Once the path pattern is updated, I'll be happy to review it again. Thanks for the contribution!
|
Hi @Rajath2005, thank you for your patience! I believe I have updated the workflow according to your feedback (using src/**/*.java and removing the failure handling step). I'm currently seeing that the branch is up to date, but if you still see any issues in the configuration, please let me know or point out exactly what might be missing. I am happy to make further adjustments if needed to get this merged! |
Rajath2005
left a comment
There was a problem hiding this comment.
The badges added to the README.md look good, but there are a few significant issues with the newly added GitHub Actions workflow (.github/workflows/ci.yaml) that should be addressed before merging:
1. Java Version Mismatch
The workflow configures the runner to use JDK 11, but if you check the project's pom.xml, maven.compiler.source and maven.compiler.target are set to 21. The workflow will likely fail or cause issues down the line.
Fix: Change java-version: '11' to java-version: '21'.
2. Incorrect Build Mechanism (Bypassing Maven)
The workflow uses a shell script to manually find and compile java files (find src -name "*.java" > sources.txt and javac @sources.txt). Because this is a Maven project, this approach is incorrect for several reasons:
- It completely ignores dependencies defined in
pom.xml(likemysql-connector-j), which will cause compilation errors if those dependencies are used in the code. - It bypasses the
maven-checkstyle-pluginconfigured in thepom.xml(which is meant to fail the build on linting errors).
Fix: Replace the manualjavacstep with standard Maven commands. For example:
- name: Build with Maven
run: mvn --batch-mode clean verify(Note: using verify is recommended here as it will trigger the Checkstyle validation configured in your POM).
3. Missing cache for Maven (Optional but Recommended)
When switching to a Maven build, the setup-java action can automatically cache dependencies to speed up subsequent runs.
Fix: Add cache: 'maven' to the setup-java step.
4. Overly Restrictive Trigger Paths
The workflow currently only triggers if a file in src/**/*.java is modified. This means if someone updates a dependency in pom.xml, or updates checkstyle.xml, the CI will not run to verify if the build is still healthy.
Fix: Consider removing the paths: filters entirely so the CI runs on all PRs to main, or at least explicitly add pom.xml and checkstyle.xml to the paths list.
Suggested ci.yaml Replacement
Here is a suggested replacement for .github/workflows/ci.yaml:
name: Java CI
on:
push:
branches: [ main ]
pull_request:
branches: [ main ]
jobs:
build:
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v4
- name: Set up JDK 21
uses: actions/setup-java@v4
with:
java-version: '21'
distribution: 'temurin'
cache: 'maven'
- name: Build with Maven
run: mvn --batch-mode clean verify
Key Changes:
Automated Build Pipeline: Created a GitHub Actions workflow to automate the compilation of the Java source code.
CI Configuration: Implemented path filtering to trigger the build process only when changes are made to Java source files (src//*.java), ensuring efficient resource usage.
Project Documentation: Integrated a build status badge into the README.md to provide immediate feedback on the project's build health.
Closes #24