[MOSIP-43541] revoked changes for develop branch #1601
Conversation
Updated the GitHub Actions workflow to trigger on release events and modified the push trigger conditions. Adjusted job dependencies and removed unnecessary steps for detecting changes. Signed-off-by: Ivanmeneges <ivan.anil016@gmail.com>
WalkthroughReplaces push branch filters with a release (published) trigger, removes the Changes
Sequence Diagram(s)sequenceDiagram
participant Release as GitHub Release
participant GH_Actions as GH Actions (push-trigger.yml)
participant Check as check_snapshot_version
participant Matrix as build_dockers (matrix)
participant Maven as maven-build@master-java21
participant DockerWF as docker-build@master-java21
Note over Release,GH_Actions: Trigger (release published) or PR/dispatch
Release->>GH_Actions: trigger workflow
GH_Actions->>Check: run check_snapshot_version
alt snapshot? true
Check-->>GH_Actions: is_snapshot = true
else
Check-->>GH_Actions: is_snapshot = false
end
GH_Actions->>Matrix: start matrix jobs (parallel per SERVICE_NAME)
Matrix->>DockerWF: call docker-build@master-java21
GH_Actions->>Maven: call maven-build@master-java21
Note right of DockerWF: builds images and produces artifacts
Note right of Maven: builds artifacts and triggers publish/sonar as needed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.github/workflows/push-trigger.yml (4)
100-101: Deprecated::set-outputsyntax - use$GITHUB_OUTPUTinstead.The
::set-outputcommand was deprecated in October 2022 and will eventually stop working. Use the new$GITHUB_OUTPUTenvironment file syntax.🔎 Proposed fix
- echo "::set-output name=is_condition::$condition_met" + echo "is_condition=$condition_met" >> $GITHUB_OUTPUT
238-239: Invalid event names in condition:prereleaseandpublishare not GitHub event types.The condition checks for
github.event_name != 'prerelease'andgithub.event_name != 'publish', but these are not valid GitHub Actions event names. For release events,github.event_nameis alwaysrelease, and the release type (published, prereleased, etc.) is ingithub.event.action.The check
github.event_name != 'release'on line 239 already covers this case, making the other checks redundant (always true).🔎 Proposed fix
- if: "${{ !contains(github.ref, 'master') && github.event_name != 'pull_request' && github.event_name != 'release' && github.event_name != 'prerelease' && github.event_name != 'publish' }}" + if: "${{ !contains(github.ref, 'master') && github.event_name != 'pull_request' && github.event_name != 'release' }}"
280-289: Logic error: Condition is always true.The condition
!contains(github.ref, 'master') || !contains(github.ref, 'main')is always true because a ref cannot contain both 'master' AND 'main' simultaneously. By De Morgan's law,!A || !Bequals!(A && B), and sinceA && Bis always false, the condition is always true.If the intent is to skip this step when on either master or main branch, use
&&instead of||:🔎 Proposed fix
- name: Ready the springboot artifacts - if: ${{ !contains(github.ref, 'master') || !contains(github.ref, 'main') }} + if: ${{ !contains(github.ref, 'master') && !contains(github.ref, 'main') }} run: | ## FIND JARS & COPY ONLY EXECUTABLE JARs STORED UNDER TARGET DIRECTORY find ${{ env.SERVICE_LOCATION }} -path '*/target/*' -exec zip ${{ env.BUILD_ARTIFACT }}.zip {} + - name: Upload the springboot jars - if: ${{ !contains(github.ref, 'master') || !contains(github.ref, 'main') }} + if: ${{ !contains(github.ref, 'master') && !contains(github.ref, 'main') }} uses: actions/upload-artifact@v4
369-380: Same logic error: Condition is always true.Same issue as in
build_apitest_esignet_local- the||should be&&to correctly exclude master and main branches.🔎 Proposed fix
- name: Ready the springboot artifacts - if: ${{ !contains(github.ref, 'master') || !contains(github.ref, 'main') }} + if: ${{ !contains(github.ref, 'master') && !contains(github.ref, 'main') }} run: | - name: Upload the springboot jars - if: ${{ !contains(github.ref, 'master') || !contains(github.ref, 'main') }} + if: ${{ !contains(github.ref, 'master') && !contains(github.ref, 'main') }} uses: actions/upload-artifact@v4
🧹 Nitpick comments (1)
.github/workflows/push-trigger.yml (1)
53-54: Consider updating to latest action versions.
actions/checkout@v3andactions/setup-java@v3have v4 versions available with improvements. This applies to lines 54, 261, 263, 344, and 346.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/push-trigger.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-19T07:14:21.109Z
Learnt from: mohanachandran-s
Repo: mosip/esignet PR: 1590
File: api-test/pom.xml:76-85
Timestamp: 2025-12-19T07:14:21.109Z
Learning: In the mosip/esignet repository, the distributionManagement release repository uses `https://central.sonatype.com/api/v1/publisher` with the central-publishing-maven-plugin (version 0.7.0), which is the correct configuration for Sonatype Central Portal API-based publishing across all modules.
Applied to files:
.github/workflows/push-trigger.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: oidc-ui / build-dockers
- GitHub Check: build-maven-uitest-esignet / maven-build
- GitHub Check: build_maven_apitest_esignet / maven-build
- GitHub Check: build_maven_esignet / maven-build
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (4)
.github/workflows/push-trigger.yml (4)
15-23: Hardcoded feature branchES-842in push triggers.The branch pattern
ES-842on line 23 appears to be a specific feature/issue branch. This is typically not included in production workflow triggers. Please verify if this is intentional or should be removed.Also, the
!release-branchnegation pattern (line 17) listed before other patterns is unusual. GitHub Actions evaluates branch filters as a union; placing a negation first might not exclude as expected if later patterns match.
110-110: Inconsistent workflow references: some jobs use@master, others use@master-java21.The workflow references have been changed from
@master-java21to@masterfor main esignet jobs (lines 110, 124, 159, 195), but api-test and ui-test jobs (lines 227, 241, 309, 401) still reference@master-java21. This inconsistency could cause build issues if Java version requirements differ.Please verify that the
@masterbranch inmosip/kattuprovides equivalent functionality for your Java version requirements.Also applies to: 124-124, 159-159
184-186: Verify dependency chain behavior with conditionalpublish_to_nexus.The
build_dockers_esignet_with_pluginsjob depends onpublish_to_nexus, which has a conditional that skips it on master branch and pull requests. Whenpublish_to_nexusis skipped, GitHub Actions will still allow dependent jobs to run (skipped jobs satisfy theneedsrequirement).However, this dependency chain seems odd - the docker build for plugins depends on publish completing, but publish doesn't run on master. Please verify this is the intended behavior for release workflows.
148-170: LGTM!The
build_dockersjob structure with matrix strategy is well-organized and allows for future extensibility. The dependency onbuild_maven_esignetensures proper build order.
Signed-off-by: Ivanmeneges <ivan.anil016@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/push-trigger.yml (1)
48-106: Replace deprecated::set-outputsyntax.Line 101 uses the deprecated
::set-outputcommand syntax. GitHub deprecated this in 2022 and recommends using environment files instead. This may stop working in future runner versions.🔎 Proposed fix
- # Set the output for the next step - echo "::set-output name=is_condition::$condition_met" + # Set the output for the next step + echo "is_condition=$condition_met" >> $GITHUB_OUTPUT
♻️ Duplicate comments (1)
.github/workflows/push-trigger.yml (1)
322-322: Security/reliability concern: Personal fork still in use.This workflow continues to reference
anup-nehe/kattuinstead of the organization repositorymosip/kattu. As noted in previous reviews, this introduces security and reliability risks.
🧹 Nitpick comments (1)
.github/workflows/push-trigger.yml (1)
1-5: Workflow name is inconsistent with trigger events.The workflow name "Maven Package upon a push" no longer accurately describes when this workflow runs, since it now triggers on release events (line 4-5) and pull requests (line 6-7) in addition to pushes.
Consider renaming the workflow to reflect all trigger types, e.g., "Maven Package and Docker Build" or "CI/CD Pipeline."
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/push-trigger.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build_maven_esignet / maven-build
- GitHub Check: oidc-ui / build-dockers
- GitHub Check: build-maven-uitest-esignet / maven-build
- GitHub Check: build_maven_apitest_esignet / maven-build
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (2)
.github/workflows/push-trigger.yml (2)
185-186: Verify job dependency chain behavior.The
build_dockers_esignet_with_pluginsjob depends onpublish_to_nexus(line 185), which only runs when the branch is not master and the event is not a pull_request (line 108). This meansbuild_dockers_esignet_with_pluginswill be skipped on master branches and pull requests, even when the SNAPSHOT condition is true.If you want this job to run independently of
publish_to_nexus, consider:
- Removing
publish_to_nexusfrom the needs array, or- Adding a conditional check that accounts for the publish_to_nexus job being skipped
27-27: Verify that themaster-java21branch exists in mosip/kattu.Multiple jobs (lines 27, 110, 124, 159, 172, 195, 227, 241, 309, 401) now reference
@master-java21instead of@master. If this branch doesn't exist or lacks the required workflow files, all dependent jobs will fail. This appears to be an intentional change for Java 21 support, but the branch existence must be confirmed to ensure the CI/CD pipeline remains functional.
Signed-off-by: Ivanmeneges <ivan.anil016@gmail.com>
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 (3)
.github/workflows/push-trigger.yml (3)
101-101: Replace deprecated::set-outputcommand.The
::set-outputcommand was deprecated by GitHub Actions in favor of writing to the$GITHUB_OUTPUTfile. This may cause warnings or failures in future runner versions.🔎 Proposed fix
- echo "::set-output name=is_condition::$condition_met" + echo "is_condition=$condition_met" >> $GITHUB_OUTPUT
280-280: Logic error in conditional: should use AND (&&) not OR (||).The conditions use
||(OR) which creates incorrect logic:
!contains(github.ref, 'master') || !contains(github.ref, 'main')This evaluates to
truefor nearly all cases. For example, if the ref is "master", the condition!contains(github.ref, 'main')istrue, so the overall expression istrueand the step runs on master (likely unintended).The intended logic appears to be "skip on master OR main", which requires AND:
🔎 Proposed fix
- if: ${{ !contains(github.ref, 'master') || !contains(github.ref, 'main') }} + if: ${{ !contains(github.ref, 'master') && !contains(github.ref, 'main') }}Apply the same fix to both occurrences (lines 280 and 285).
Alternatively, use De Morgan's law:
if: ${{ !(contains(github.ref, 'master') || contains(github.ref, 'main')) }}Also applies to: 285-285
370-370: Logic error in conditional: should use AND (&&) not OR (||).Same issue as in the
build_apitest_esignet_localjob - the conditions use||(OR) instead of&&(AND), causing the steps to run on master/main branches when they should be skipped.🔎 Proposed fix
- if: ${{ !contains(github.ref, 'master') || !contains(github.ref, 'main') }} + if: ${{ !contains(github.ref, 'master') && !contains(github.ref, 'main') }}Apply to both occurrences (lines 370 and 376).
Also applies to: 376-376
🧹 Nitpick comments (2)
.github/workflows/push-trigger.yml (2)
56-101: Consider using Maven commands instead of parsing XML.The bash script parses
pom.xmlfiles to detect SNAPSHOT versions in parent blocks. While the implementation appears correct, parsing XML with text tools can be fragile. Consider using Maven's built-in capabilities for more robust version detection:Alternative approach using Maven
- name: Check for -SNAPSHOT version in pom.xml files id: check_output shell: bash run: | condition_met=false for pom_file in $(find . -name "pom.xml"); do echo "Processing $pom_file" parent_version=$(mvn help:evaluate -Dexpression=project.parent.version -q -DforceStdout -f "$pom_file" 2>/dev/null || echo "") if [[ "$parent_version" == *"-SNAPSHOT"* ]]; then echo "Found '-SNAPSHOT' version in $pom_file: $parent_version" condition_met=true break fi done echo "is_condition=$condition_met" >> $GITHUB_OUTPUTThis approach uses Maven's expression evaluation which handles XML parsing and Maven property interpolation correctly.
1-1: Consider updating workflow name to reflect all trigger types.The workflow name "Maven Package upon a push" doesn't reflect that it also runs on release events, pull requests, and manual triggers. Consider a more generic name like "Build and Test Pipeline" or "CI/CD Pipeline".
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/push-trigger.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (4)
.github/workflows/push-trigger.yml (4)
215-215: Inconsistent workflow reference uses@masterinstead of@master-java21.The
build_dockers_oidc_uijob references the docker-build workflow at@master, while most other jobs in this file use@master-java21. This inconsistency could lead to compatibility issues or unexpected behavior if the two branches have diverged.Verify whether this should use
@master-java21for consistency or if@masteris intentionally required for the OIDC UI service.
322-322: Inconsistent workflow reference uses@developinstead of@master-java21.The
build-maven-uitest-esignetjob references the maven-build-browserStack workflow at@develop, while most other jobs use@master-java21. This creates inconsistency in the workflow dependencies and may indicate:
- The browserStack workflow hasn't been updated to support Java 21 yet
- This is temporary until the workflow is promoted to a stable branch
- There's a specific reason this test requires the develop branch
Verify the intentionality of this reference and consider adding a comment explaining why this job uses a different branch reference than the others.
Based on learnings, a past review flagged security concerns about using personal forks. While this now correctly uses the organization repository (
mosip/kattu), using the@developbranch in production CI/CD may introduce instability compared to using stable branch references.
4-5: Verify release trigger compatibility with job conditions.The workflow now includes a
releasetrigger withtypes: [published], but several jobs have conditions that may need review:
- Line 108:
publish_to_nexusruns if NOT master AND NOT pull_request (will run on release events)- Line 239:
publish_to_nexus_apitest_esignetexplicitly excludes 'release', 'prerelease', and 'publish' eventsVerify that the intended behavior for release events is correctly implemented across all conditional jobs. The explicit exclusion at line 239 suggests release events should not trigger nexus publishing for api-test, but the job at line 108 will run on release events.
Also applies to: 108-108, 239-239
27-27: Verifymaster-java21branch access and workflow compatibility in mosip/kattu.Multiple jobs reference workflows from the mosip/kattu repository at the
@master-java21branch (lines 27, 110, 124, 159, 172, 195, 227, 241, 309, 401). While MOSIP documentation mentions this branch as part of Java-21 migration efforts, verify that:
- The
master-java21branch is accessible in the mosip/kattu repository- All referenced workflows (maven-build.yml, maven-publish-to-nexus.yml, maven-sonar-analysis.yml, docker-build.yml) exist at that branch with compatible interfaces
- The branch is stable and appropriate for production use
Note: Some jobs still reference
@develop(lines 39, 139, 322) or@master(line 215), creating mixed branch dependencies that may complicate maintenance.
Signed-off-by: Ivanmeneges <ivan.anil016@gmail.com>
2 participants
Updated the GitHub Actions workflow to trigger on release events and modified the push trigger conditions. Adjusted job dependencies and removed unnecessary steps for detecting changes.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.