refine workflow #34
Merged
refine workflow #34
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Warning Rate limit exceeded@Tearran has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 38 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
## Walkthrough
This update removes obsolete GitHub Actions workflows and scripts, introduces an enhanced staging validation workflow, strengthens module validation logic, and updates configuration files for submenu and yes_no_box features to explicitly specify options and helpers. The main validation script now enforces stricter checks and broader duplicate detection.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------|
| .github/workflows/00_github_staging-validate.yml | Deleted old staging validation workflow. |
| .github/workflows/03_github_staging-validate.yml | Added new, more comprehensive staging validation workflow with enhanced script execution and reporting. |
| .github/workflows/05_github_tests-validate.yml.hold | Deleted test validation workflow for shell-based tests. |
| tools/02_validate_module.sh | Improved validation: stricter failure reporting, new required field (`options`), broader duplicate detection, logic refactor. |
| tools/04_promote_module.sh.hold | Deleted script for promoting modules from staging to target directories. |
| src/core/interface/submenu.conf, src/core/interface/yes_no_box.conf | Updated configuration files to explicitly specify options and helper functions. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant GitHub Actions
participant Runner
participant tools/02_validate_module.sh
participant tools/[NN]_*.sh
GitHub Actions->>Runner: Trigger on PR modifying staging/ or tools/
Runner->>tools/02_validate_module.sh: Validate staging directory and modules
tools/02_validate_module.sh-->>Runner: Fail if missing/invalid, else continue
Runner->>tools/[NN]_*.sh: For each script (except 02_validate_module.sh), execute
tools/[NN]_*.sh-->>Runner: Return pass/fail status
Runner-->>GitHub Actions: Report overall pass/failPossibly related PRs
Poem
|
deleted: staging/foo.conf deleted: staging/foo.md deleted: staging/foo.sh renamed: src/core/interface/submenu.conf -> staging/submenu.conf renamed: docs/submenu.md -> staging/submenu.md renamed: src/core/interface/submenu.sh -> staging/submenu.sh
new file: src/core/interface/submenu.sh
deleted: src/core/interface/submenu.conf deleted: src/core/interface/submenu.sh renamed: src/core/interface/yes_no_box.conf -> staging/yes_no_box.conf renamed: docs/yes_no_box.md -> staging/yes_no_box.md renamed: src/core/interface/yes_no_box.sh -> staging/yes_no_box.sh
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tools/02_validate_module.sh (1)
82-90: Unreachable and duplicate empty-value handling foroptionsThe generic empty-value check at lines 82–90 triggers a
continuefor all fields (includingoptions), so the specificoptionscase in thecaseblock never executes. Consolidate the empty check foroptionsinto the generic block or reorder logic:- if [ -z "$value" ]; then - if [ "$field" = "helpers" ]; then - failed_fields+=(" helpers (no helper listed; must have at least _about_$feature)") - else - failed_fields+=(" $field (empty)") - fi - failed=1 - continue - fi + if [ -z "$value" ]; then + case "$field" in + helpers) + failed_fields+=(" helpers (no helper listed; must have at least _about_$feature)");; + options) + failed_fields+=(" options (blank; should describe supported options or 'none')");; + *) + failed_fields+=(" $field (empty)");; + esac + failed=1 + continue + fiThen remove the redundant empty check under the
options)case.
🧹 Nitpick comments (2)
tools/02_validate_module.sh (2)
139-158: Optional: Simplify duplicate detection with a singlefindinvocationThe nested loops over
./src/./docsand.sh/.md/.confwork but can be simplified for readability and performance. For example:- _check_duplicate_anywhere() { - local modname="$1"; local found=0 - for dir in ./src ./docs; do - for ext in .sh .md .conf; do - while IFS= read -r file; do - [[ -z "$file" || "$file" == ./staging/* ]] && continue - echo "FAIL: Duplicate found: $file"; found=1 - done < <(find "$dir" -type f -name "$modname$ext") - done - done - return $found - } + _check_duplicate_anywhere() { + local modname="$1"; local found=0 + while IFS= read -r file; do + echo "FAIL: Duplicate found: $file"; found=1 + done < <(find ./src ./docs \ + -type f \( -name "${modname}.sh" -o -name "${modname}.md" -o -name "${modname}.conf" \) \ + -not -path "./staging/*") + return $found + }This merges the searches and uses
-not -pathto skip staging.
185-187: Enhance unknown-command handling with usage outputOn an unknown command you exit with an error but don’t guide the user. Consider printing the usage help:
- *) - echo "Unknown command" >&2 - exit 1 + *) + echo "Unknown command: $cmd" >&2 + _about_validate_module + exit 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/02_validate_module.sh(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
tools/02_validate_module.sh (1)
Learnt from: Tearran
PR: Tearran/configng-v2#25
File: tools/04_promote_module.sh:30-38
Timestamp: 2025-06-18T05:04:30.676Z
Learning: In tools/04_promote_module.sh, the user wants comprehensive validation of configuration files before promotion, checking required fields like 'feature', 'helpers', 'description', and 'parent' rather than just 'parent'. This maintains fail-fast behavior while ensuring only properly configured modules are promoted.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tools/02_validate_module.sh (1)
7-19: Update usage help to include 'options' fieldThe usage message still lists required
.conffields without specifyingoptions, which is now enforced by the script. Please update the help text to reflect all required non-comment fields:feature, options, helpers, description, parent, group, contributor, maintainer, port.
🧹 Nitpick comments (1)
tools/02_validate_module.sh (1)
182-185: Enhance unknown command handlingInstead of just printing "Unknown command", consider echoing the invalid input and showing the usage help for clarity:
@@ 182,4 +182,6 - echo "Unknown command" >&2 - exit 1 + echo "Unknown command: $cmd" >&2 + _about_validate_module + exit 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/02_validate_module.sh(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
tools/02_validate_module.sh (1)
Learnt from: Tearran
PR: Tearran/configng-v2#25
File: tools/04_promote_module.sh:30-38
Timestamp: 2025-06-18T05:04:30.676Z
Learning: In tools/04_promote_module.sh, the user wants comprehensive validation of configuration files before promotion, checking required fields like 'feature', 'helpers', 'description', and 'parent' rather than just 'parent'. This maintains fail-fast behavior while ensuring only properly configured modules are promoted.
🔇 Additional comments (5)
tools/02_validate_module.sh (5)
33-40: Consistent failure messaging in markdown checkThe enhanced checks use
FAIL:prefixes for missing or incomplete headers, which aligns with the script's fail-fast policy. No changes needed here.
51-56: Consistent failure messaging in shell checkThe updated shell-file check now correctly emits
FAIL:when_about_<modulename>()is missing. This is consistent with other checks.
58-63: Expanded REQUIRED_CONF_FIELDS declarationAdding
options,group,contributor,maintainer, andportstrengthens configuration validation and aligns with your staging.conffiles.
138-156: Duplicate detection logic is robustThe new
_check_duplicate_anywherefunction correctly scans./srcand./docs, excludes./staging, and usesfoundto return a non-zero exit status on duplicates. This fulfills the requirement for comprehensive duplicate checks.
168-176: Integrated duplicate check in module validationYou’ve correctly plugged the new duplicate check into the
allbranch, preserving fail-fast behavior when duplicates are found.
renamed: tools/setup_module.sh -> tools/start_here.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tools/02_validate_module.sh (2)
59-64: Consider strengthening 'port' validation
portis required but only checked for non-emptiness. You may want to ensure it's a valid integer (and optionally constrain it to a known range) for better correctness.
143-221: Remove obsolete_check_conf_refatoringfunction
This unused refactoring block duplicates_check_confand is never called. Removing it will reduce confusion and improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/02_validate_module.sh(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
tools/02_validate_module.sh (1)
Learnt from: Tearran
PR: Tearran/configng-v2#25
File: tools/04_promote_module.sh:30-38
Timestamp: 2025-06-18T05:04:30.676Z
Learning: In tools/04_promote_module.sh, the user wants comprehensive validation of configuration files before promotion, checking required fields like 'feature', 'helpers', 'description', and 'parent' rather than just 'parent'. This maintains fail-fast behavior while ensuring only properly configured modules are promoted.
🔇 Additional comments (6)
tools/02_validate_module.sh (6)
11-13: Unified 'FAIL' status in usage
Switching the incomplete-file message from "WARN" to "FAIL" aligns with the strict fail-fast approach.
33-35: Consistent failure prefix for missing header in Markdown checks
Changing the warning to a failure here reinforces the script’s fail-fast design.
37-39: Consistent failure prefix for single-header Markdown check
Reporting a lone top-level header as a failure is now consistent with other checks.
51-53: Consistent failure prefix for missing shell help function
Marking a missing_about_<modname>()as a failure now aligns with the updated validation policy.
223-243: Duplicate-detection function rewrite looks solid
The new_check_duplicate_anywherecorrectly scanssrc/anddocs/, excludes./staging, and treats duplicates as failures.
259-262: Integrated duplicate check in main loop
Invoking_check_duplicate_anywherewithin the main validation loop ensures duplicate files block the promotion path as intended.
Comment on lines
16
to
18
| - <modulename>.sh (must contain Help info in _about_<modulename>() function) | ||
| - <modulename>.conf (must have required non-comment fields: feature, helpers, description) | ||
| - Checks for duplicate-named files in src/** (warns if present) | ||
| - Checks for duplicate-named files in src/** and docs/** (outside of ./staging) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update .conf usage to reflect additional required fields
The usage text still lists only feature, helpers, and description. Since validation now requires options, parent, group, contributor, maintainer, and port, please update this line to reflect the full set.
🤖 Prompt for AI Agents
In tools/02_validate_module.sh around lines 16 to 18, the usage text for .conf
files lists only the fields feature, helpers, and description, but validation
now requires additional fields: options, parent, group, contributor, maintainer,
and port. Update the usage text line to include all these required non-comment
fields to accurately reflect the current validation requirements.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request refines the workflow by updating the staging validation process, removing obsolete scripts and workflows, and enhancing the validation checks for modules.
- Replaces the old staging and test validation workflows with improved validation logic and duplicate detection.
- Updates configuration files (submenu and yes/no box) to clarify helper and options metadata.
- Removes obsolete scripts and workflows no longer in use.
Reviewed Changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/04_promote_module.sh.hold | Removed obsolete script for promoting modules from staging. |
| tools/02_validate_module.sh | Updated validation messages to report failures instead of warnings and replaced duplicate check with an expanded version. |
| src/core/interface/yes_no_box.conf | Updated the helpers field to explicitly reference the helper function. |
| src/core/interface/submenu.conf | Updated the options field and the helpers field for clarity. |
| .github/workflows/05_github_tests-validate.yml.hold | Removed obsolete test workflow. |
| .github/workflows/03_github_staging-validate.yml | Added new staging gate workflow with enhanced validation logic. |
| .github/workflows/00_github_staging-validate.yml | Removed outdated staging workflow. |
Comments suppressed due to low confidence (3)
tools/02_validate_module.sh:191
- [nitpick] Consider providing additional context in the error message to guide the user on acceptable commands.
echo "Unknown command" >&2
src/core/interface/submenu.conf:8
- [nitpick] Ensure that the new placeholder text for options is clearly explained in accompanying documentation for user clarity.
options=<module_name help>
src/core/interface/yes_no_box.conf:17
- [nitpick] Confirm that the helper function _about_yes_no_box is properly documented for consistent interface usage.
helpers=_about_yes_no_box
This was referenced Jun 23, 2025
Merged
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit