[MOSIP-44751]fix :helmsman workflows always deploy java11 profile regardless of selected profile#189
Conversation
Merge pull request mosip#173 from bhumi46/develop
…ss of selected profile Signed-off-by: bhumi46 <thisisbn46@gmail.com>
WalkthroughUpdated GitHub Actions workflow profiles with enhanced auto-detection logic: added fallback git diff operations when initial profile detection fails, replaced silent defaults with explicit error handling that aborts on missing profiles, extended job outputs in external workflow, and updated dependencies to use matrix-generated profile values. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/helmsman_external.yml (1)
68-74: Consider edge case: multiple profiles changed simultaneously.If changes span multiple profile directories (e.g., both
mosip-platform-java11/andmosip-platform-java21/), the matrix correctly includes DSF files from all profiles, butPROFILEoutput (line 70) only captures the first one. This first profile is then passed to the downstream MOSIP workflow (line 281).For typical use cases this should be fine since changes usually target a single profile, but worth noting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/helmsman_external.yml around lines 68 - 74, The current logic sets PROFILE=$(echo "$changed_profiles" | head -1) which only picks the first profile when multiple profiles are changed; update the logic that derives PROFILE from changed_profiles to handle multiple profiles (either by exporting a comma/newline-separated list or by erroring out if multi-profile changes are unsupported) and ensure the downstream MOSIP workflow invocation consumes that full list (not just the first element). Specifically, replace the head -1 usage on changed_profiles and set PROFILE to the entire set (e.g., CSV or array) or add a guard that fails with a clear message when multiple profiles are detected, and update the downstream dispatch/inputs to accept the list instead of a single PROFILE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/helmsman_external.yml:
- Around line 68-74: The current logic sets PROFILE=$(echo "$changed_profiles" |
head -1) which only picks the first profile when multiple profiles are changed;
update the logic that derives PROFILE from changed_profiles to handle multiple
profiles (either by exporting a comma/newline-separated list or by erroring out
if multi-profile changes are unsupported) and ensure the downstream MOSIP
workflow invocation consumes that full list (not just the first element).
Specifically, replace the head -1 usage on changed_profiles and set PROFILE to
the entire set (e.g., CSV or array) or add a guard that fails with a clear
message when multiple profiles are detected, and update the downstream
dispatch/inputs to accept the list instead of a single PROFILE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6aa30467-87e1-495c-86d9-8a05c0ab1de3
📒 Files selected for processing (4)
.github/workflows/helmsman_esignet.yml.github/workflows/helmsman_external.yml.github/workflows/helmsman_mosip.yml.github/workflows/helmsman_testrigs.yml
Summary by CodeRabbit
Release Notes