Add reusable push_to_review_firm workflow#24
Open
Benjvandam wants to merge 2 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds a reusable workflow that resolves a Silverfin review firm, finds matching development PRs, pushes changed templates with silverfin-cli, and posts status comments. It also updates the README with usage and behavior details. Changespush-to-review-firm workflow
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)README.mdmarkdownlint-cli2 wrapper config was not available before execution 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: 3
🧹 Nitpick comments (1)
README.md (1)
49-50: ⚡ Quick winMake the new table-of-contents entries clickable for consistency.
The new TOC entries are plain text while the rest are links, which hurts navigation.
Suggested diff
- * Review firm deployment - * Push templates to review firm (push_to_review_firm.yml) + * [Review firm deployment](`#review-firm-deployment`) + * [Push templates to review firm (push_to_review_firmyml)](`#push-templates-to-review-firm-push_to_review_firmyml`)🤖 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 `@README.md` around lines 49 - 50, The new TOC lines "Review firm deployment" and "Push templates to review firm (push_to_review_firm.yml)" are plain text; change them to Markdown links consistent with the rest of the TOC by wrapping the display text in [ ] and adding the target anchors or file paths in ( ), e.g., convert the "Review firm deployment" entry and the "Push templates to review firm (push_to_review_firm.yml)" entry into clickable links pointing to the corresponding section anchor or the push_to_review_firm.yml file so they match the rest of the table-of-contents style.
🤖 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/push_to_review_firm.yml:
- Around line 227-236: The script currently uses jq to extract IDVAL but then
silently falls back to basename if jq fails (e.g., malformed config.json);
modify the assignments inside the case (the lines setting IDVAL=$(jq -r '...'
"${DIR}/config.json")) to detect jq failures and fail fast: run jq and if it
exits non-zero or returns an explicit parse error, set PUSHED["${DIR}"]="❌
malformed config.json", set ANY_FAILED=1 and continue to the next item instead
of falling back to basename; reference the IDVAL and DIR variables and the case
branch that sets IDVAL so you add an immediate check (e.g., use "IDVAL=$(jq -r
'...' file) || { ... continue; }") before the existing basename fallback.
- Around line 197-200: The workflow currently silently continues when git
checkout -f "origin/${REF}" fails inside the PR loop, which can lead to false
success; update the failure branch for the checkout command (the if that checks
git checkout -f "origin/${REF}" --quiet) to emit a clear error (include the PR
identifier ${NUM} and ref ${REF} via ::error:: or echo), close the group
(::endgroup::), and terminate the job with a non-zero exit (exit 1) instead of
using continue so the workflow reports failure rather than skipping the PR.
In `@README.md`:
- Line 202: Update the product name casing in the README sentence that currently
reads "The Github action will create a text object…" to use the correct "GitHub"
capitalization; locate that line in README.md and replace "Github" with "GitHub"
so the user-facing text displays the proper product name.
---
Nitpick comments:
In `@README.md`:
- Around line 49-50: The new TOC lines "Review firm deployment" and "Push
templates to review firm (push_to_review_firm.yml)" are plain text; change them
to Markdown links consistent with the rest of the TOC by wrapping the display
text in [ ] and adding the target anchors or file paths in ( ), e.g., convert
the "Review firm deployment" entry and the "Push templates to review firm
(push_to_review_firm.yml)" entry into clickable links pointing to the
corresponding section anchor or the push_to_review_firm.yml file so they match
the rest of the table-of-contents style.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48b97c74-e360-466f-ab03-2123b2950d5a
📒 Files selected for processing (2)
.github/workflows/push_to_review_firm.ymlREADME.md
| echo "::endgroup::" | ||
| done | ||
|
|
||
| if [ "${ANY_SHARED_PART}" -eq 1 ]; then |
Collaborator
There was a problem hiding this comment.
🟠 Major — add-shared-part --all runs after the PR loop against whichever tree the last successful git checkout left on disk, so in multi-PR runs shared-part linkage from earlier PRs may be incomplete even when those shared parts were pushed successfully.
# Option A: run add-shared-part --all inside each PR loop after pushing that PR's shared parts
# Option B: re-checkout each PR that touched shared_parts before a final link pass
Author
There was a problem hiding this comment.
Done — moved it inside the PR loop so it runs against the right tree for each PR.
| continue | ||
| fi | ||
|
|
||
| DIRS=$(git diff --name-only "origin/main...origin/${REF}" 2>/dev/null | grep -oE "${PATTERN}" | sort -u || true) |
Collaborator
There was a problem hiding this comment.
🟡 Minor — The diff base is hardcoded to origin/main; market repos whose default branch is not main will get wrong (or empty) change detection.
BASE="${GITHUB_EVENT_REPOSITORY_DEFAULT_BRANCH:-main}"
DIRS=$(git diff --name-only "origin/${BASE}...origin/${REF}" ...)
Author
There was a problem hiding this comment.
Done — now uses the repo default branch instead of hardcoded main.
- Fail the run on PR checkout failure instead of silently skipping - Run add-shared-part --all per PR, against each PR's checked-out tree - Resolve diff base from the repo default branch instead of hardcoded main - Capture update/create exit code under set -e (bash -eo pipefail) - README: GitHub casing + clickable TOC links
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
Add a reusable
push_to_review_firm.ymlworkflow. A market repo wraps it withrepository_dispatch(fired by a Jira Automation button on a functional-review ticket) so a product manager can push the templates from the linked development PR(s) to their Silverfin review firm with one click. It is the dispatch-driven, multi-PR, parameterised generalisation ofupdate_templates_review.yml.Smoke-tested end-to-end from a temporary wrapper in
silverfin/be_marketagainst PR #2850 (BE-14175) into firm6056: the reusable matched the PR, diffed againstmain, updatedreconciliation_texts/vol_10on firm 6056, and posted a status comment on the PR. Run: https://github.com/silverfin/be_market/actions/runs/25790165124The market-repo wrapper and the Jira Automation rule are follow-ups. The first market wrapper will be opened against
silverfin/be_marketand pinned to this branch until this PR merges.Reader-only on
CONFIG_JSON: this workflow never refreshes tokens and never writes the secret back, so it does not become a concurrent writer. It relies on the existing refresher to keep the secret fresh — see docs/github_actions_authentication.md.Type of change
Checklist