Make start-translation fail loud instead of exiting 0 when submodule discovery finds nothing. - #69
Conversation
…discovery finds nothing
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe workflow validates ChangesSubmodule synchronization reliability
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Gitmodules
participant sync-mirrors
participant start-local
Gitmodules->>sync-mirrors: Validate libs/ submodule paths
sync-mirrors->>start-local: Provide updated_submodules handoff
start-local->>start-local: Fail if submodule_names is empty
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
tests/test_submodule_ops.bats (1)
39-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a multi-entry
pipefailregression test.The success fixture has only one
libs/path, so it cannot catch the validator’s early-grep -qfailure. Add a generated, sufficiently large set oflibs/...entries and run the test withset -o pipefail.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_submodule_ops.bats` around lines 39 - 62, Expand the success test for require_libs_submodules_in_gitmodules with a generated, sufficiently large number of libs/... submodule entries, and enable set -o pipefail while running the validator. Keep the existing successful status assertion so the test detects premature grep -q pipeline failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/assets/submodule_ops.sh:
- Line 53: Update the submodule-name check around
libs_submodule_names_from_gitmodules_file to capture its complete output before
testing whether it is empty, instead of piping directly to grep -q. Preserve the
existing failure behavior for an empty result while avoiding producer SIGPIPE
under pipefail.
---
Nitpick comments:
In `@tests/test_submodule_ops.bats`:
- Around line 39-62: Expand the success test for
require_libs_submodules_in_gitmodules with a generated, sufficiently large
number of libs/... submodule entries, and enable set -o pipefail while running
the validator. Keep the existing successful status assertion so the test detects
premature grep -q pipeline failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90868249-adc8-4348-ac36-ad5e97995ba1
📒 Files selected for processing (4)
.github/workflows/assets/submodule_ops.sh.github/workflows/start-translation.ymldocs/ARCHITECTURE.mdtests/test_submodule_ops.bats
wpak-ai
left a comment
wpak-ai
left a comment
There was a problem hiding this comment.
.github/workflows/assets/lib.sh:166: make ensure_local_branch_in_translations propagate each git command's status explicitly, for example git -C "$dir" push -u origin "$branch" || return 1, and add an explicit return 0 after the trailing echo
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_submodule_ops.bats (1)
39-72: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the truly empty/no-path
.gitmodulescase.Ensure the failure fixture includes a file with no
submodule.*.pathentries, then assert the same non-zero status and diagnostic. This exercises thegit config --get-regexpno-match status that can be exposed bypipefail.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_submodule_ops.bats` around lines 39 - 72, Update the failure test for require_libs_submodules_in_gitmodules to use an empty or no-path .gitmodules fixture with no submodule.*.path entries. Keep the assertion for non-zero status and the existing “Run add-submodules first.” diagnostic so the no-match pipefail behavior is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_submodule_ops.bats`:
- Around line 39-72: Update the failure test for
require_libs_submodules_in_gitmodules to use an empty or no-path .gitmodules
fixture with no submodule.*.path entries. Keep the assertion for non-zero status
and the existing “Run add-submodules first.” diagnostic so the no-match pipefail
behavior is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05444b66-2d99-4d7a-9d69-75be9f0b6982
📒 Files selected for processing (3)
.github/workflows/assets/lib.sh.github/workflows/assets/submodule_ops.shtests/test_submodule_ops.bats
3 participants
Close #64.
Summary by CodeRabbit
libs/entries are present in.gitmodules.libs/validation and for marking an update-recording failure as fatal for the affected submodule.