Add reusable push_to_review_firm workflow#24
Open
Benjvandam wants to merge 1 commit into
Open
Conversation
WalkthroughThis pull request introduces a new reusable GitHub Actions workflow that validates a target Silverfin review firm, discovers related development PRs, deduplicates changed templates across those PRs, and pushes template updates or creates new templates on the firm using silverfin-cli, with idempotent result comments and documentation. Changespush-to-review-firm workflow
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
Comment on lines
+197
to
+200
| if ! git checkout -f "origin/${REF}" --quiet 2>/dev/null; then | ||
| echo "Could not check out origin/${REF} — skipping PR #${NUM}." | ||
| echo "::endgroup::" | ||
| continue |
There was a problem hiding this comment.
Do not silently skip PRs that fail checkout.
If a matched PR cannot be checked out, the workflow currently continues and can still report success with incomplete deployment.
Suggested diff
if ! git checkout -f "origin/${REF}" --quiet 2>/dev/null; then
- echo "Could not check out origin/${REF} — skipping PR #${NUM}."
+ echo "::error::Could not check out origin/${REF} (PR #${NUM})."
+ ANY_FAILED=1
echo "::endgroup::"
continue
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ! git checkout -f "origin/${REF}" --quiet 2>/dev/null; then | |
| echo "Could not check out origin/${REF} — skipping PR #${NUM}." | |
| echo "::endgroup::" | |
| continue | |
| if ! git checkout -f "origin/${REF}" --quiet 2>/dev/null; then | |
| echo "::error::Could not check out origin/${REF} (PR #${NUM})." | |
| ANY_FAILED=1 | |
| echo "::endgroup::" | |
| continue |
🤖 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 @.github/workflows/push_to_review_firm.yml around lines 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.
Comment on lines
+227
to
+236
| case "${DIR}" in | ||
| reconciliation_texts/*) TYPE="reconciliation"; FLAG="--handle"; IDVAL=$(jq -r '.handle // empty' "${DIR}/config.json");; | ||
| shared_parts/*) TYPE="shared-part"; FLAG="--shared-part"; IDVAL=$(jq -r '.name // empty' "${DIR}/config.json");; | ||
| account_templates/*) TYPE="account-template"; FLAG="--name"; IDVAL=$(jq -r '.name_nl // .name // empty' "${DIR}/config.json");; | ||
| export_files/*) TYPE="export-file"; FLAG="--name"; IDVAL=$(jq -r '.name_nl // .name // empty' "${DIR}/config.json");; | ||
| *) PUSHED["${DIR}"]="❌ unknown template type"; ANY_FAILED=1; continue;; | ||
| esac | ||
| if [ -z "${IDVAL}" ] || [ "${IDVAL}" = "null" ]; then | ||
| IDVAL=$(basename "${DIR}") | ||
| fi |
There was a problem hiding this comment.
Fail fast on malformed config.json before deriving identifiers.
Right now a JSON parse failure can degrade into basename fallback and push/update the wrong template identity.
Suggested diff
if [ ! -f "${DIR}/config.json" ]; then
# Whole template directory removed in the PR (or no config.json) — nothing to push, not a failure.
PUSHED["${DIR}"]="⚠️ no config.json — skipped (template removed?)"
echo "${DIR}: no config.json — skipped"
continue
fi
+ if ! jq -e . "${DIR}/config.json" >/dev/null 2>&1; then
+ PUSHED["${DIR}"]="❌ invalid config.json"
+ ANY_FAILED=1
+ echo "${DIR}: invalid config.json"
+ continue
+ fi
case "${DIR}" in
reconciliation_texts/*) TYPE="reconciliation"; FLAG="--handle"; IDVAL=$(jq -r '.handle // empty' "${DIR}/config.json");;🤖 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 @.github/workflows/push_to_review_firm.yml around lines 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.
| * This workflow will then generate a URL, which is called a webhook. | ||
| * The webhook from the workflow should be stored in an environment variable in the market specific repository: `SLACK_WEBHOOK_URL` | ||
| * The Github action will create a text object (containing the formatted message) and will post this to the Slack webhook. This will then create the message in a pre-defined channel. No newline at end of file | ||
| * The Github action will create a text object (containing the formatted message) and will post this to the Slack webhook. This will then create the message in a pre-defined channel. |
There was a problem hiding this comment.
Fix product name casing (“GitHub”).
This is a user-facing typo.
Suggested diff
-* The Github action will create a text object (containing the formatted message) and will post this to the Slack webhook. This will then create the message in a pre-defined channel.
+* The GitHub action will create a text object (containing the formatted message) and will post this to the Slack webhook. This will then create the message in a pre-defined channel.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * The Github action will create a text object (containing the formatted message) and will post this to the Slack webhook. This will then create the message in a pre-defined channel. | |
| * The GitHub action will create a text object (containing the formatted message) and will post this to the Slack webhook. This will then create the message in a pre-defined channel. |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~202-~202: The official name of this software platform is spelled with a capital “H”.
Context: ...c repository: SLACK_WEBHOOK_URL * The Github action will create a text object (conta...
(GITHUB)
🤖 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` at 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.
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
1 participant
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