feat(openfga): extract model DSL to files/model.fga#136
Open
feat(openfga): extract model DSL to files/model.fga#136
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughExtracts the OpenFGA DSL into ChangesOpenFGA model extraction and CI enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 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: 1
🧹 Nitpick comments (1)
.github/workflows/fga-model-check.yml (1)
22-22: 💤 Low value
strip()may exit non-zero if input contains only comments/blanks.If the model file contains only comments and blank lines, the second
grep -vwill produce no output and return exit code 1. While this is currently absorbed by command substitution, it could cause issues if the script is later modified to useset -eor if the behavior changes.♻️ Defensive fix
- strip() { grep -v '^[[:space:]]*#' | grep -v '^[[:space:]]*$'; } + strip() { grep -v '^[[:space:]]*#' | grep -v '^[[:space:]]*$' || true; }🤖 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/fga-model-check.yml at line 22, The strip() helper can return a non-zero exit status when the input is only comments/blanks; change the implementation of strip() so the pipeline never fails (for example append a fallback like "|| true" to the grep pipeline or use a single awk/sed command that always exits 0) so that calling strip() always returns success; update the strip() function definition accordingly to ensure safe behavior under set -e.
🤖 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/fga-model-check.yml:
- Around line 22-28: git show "$BASE:$MODEL" can fail when model.fga doesn't
exist on the base branch; update the script around the BASE_DSL/HEAD_DSL
assignment to first test for the base file's existence (e.g., with git cat-file
-e "$BASE:$MODEL" or git ls-tree "$BASE" -- "$MODEL") and only run git show
"$BASE:$MODEL" if it exists, otherwise set BASE_DSL to an empty string (or a
sentinel) so the subsequent comparison with HEAD_DSL treats a newly added model
as "changed" rather than failing the workflow; keep HEAD_DSL logic the same and
still pipe through strip().
---
Nitpick comments:
In @.github/workflows/fga-model-check.yml:
- Line 22: The strip() helper can return a non-zero exit status when the input
is only comments/blanks; change the implementation of strip() so the pipeline
never fails (for example append a fallback like "|| true" to the grep pipeline
or use a single awk/sed command that always exits 0) so that calling strip()
always returns success; update the strip() function definition accordingly to
ensure safe behavior under set -e.
🪄 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: c595a67b-8556-4d59-8846-9b3b08bd09a8
📒 Files selected for processing (4)
.github/workflows/fga-model-check.ymlcharts/lfx-platform/files/model.fgacharts/lfx-platform/templates/openfga/model.yamldocs/openfga.md
Move the FGA authorization model DSL out of the inline Helm template and into a standalone file at charts/lfx-platform/files/model.fga (single source of truth). model.yaml now uses .Files.Get to include it. Add a CI workflow that fails PRs which change the model without bumping the version. Update docs to reference the new canonical location. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-1646 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
- Add heuristic caveat comment to fga-model-check CI - Comment indent vs nindent choice in model.yaml - Fix docs code example note about column-0 template line 🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-1646 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
Strip comment and blank lines before comparing model DSL so that annotation-only updates (@fgadoc:jtbd, @fgadoc:alias, @fgadoc:hide) do not require a version bump, matching the policy documented in model.yaml. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-1646 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
- Pipe directly into strip() instead of passing /dev/stdin, which is unreliable in subshells - Replace \s with [[:space:]] for POSIX portability 🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-1646 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
Add committee_for_member_roster_access, committee_for_member_email_access, roster_viewer, and email_viewer relations merged in #130. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-1646 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
bramwelt
force-pushed
the
tbramwell/LFXV2-1646-extract-fga-model
branch
from
fe9d512 to
43d722a
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the OpenFGA authorization model so the DSL lives in a standalone chart file instead of being embedded inline in the Helm template, and adds documentation/CI around that workflow. It fits the codebase by separating the model source from deployment metadata while keeping the chart-rendered AuthorizationModelRequest shape the same.
Changes:
- Extracted the OpenFGA DSL into
charts/lfx-platform/files/model.fgaand updated the Helm template to load it with.Files.Get. - Updated
docs/openfga.mdto describe the new edit flow and the template indirection. - Added a new
fga-model-checkGitHub Actions workflow intended to guard model updates.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
docs/openfga.md |
Updates operator/model maintenance docs to reference the extracted DSL file. |
charts/lfx-platform/templates/openfga/model.yaml |
Replaces the inline authorization model block with a .Files.Get include. |
charts/lfx-platform/files/model.fga |
New standalone source file containing the OpenFGA model DSL and @fgadoc annotations. |
.github/workflows/fga-model-check.yml |
Adds PR-time checks around OpenFGA model edits and version-update enforcement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review comments from coderabbitai[bot], copilot-pull-request-reviewer[bot]: - .github/workflows/fga-model-check.yml: handle missing model.fga on base branch — git show now uses 2>/dev/null with fallback so first- time additions are treated as changed rather than failing CI (per coderabbitai[bot]) - .github/workflows/fga-model-check.yml: add || true to strip() so comment/blank-only model files don't produce non-zero exits (per coderabbitai[bot]) - .agents/skills/render-permissions/SKILL.md: update to read files/model.fga directly instead of parsing the authorizationModel block in model.yaml (per copilot-pull-request-reviewer[bot]) - .agents/skills/populate-jtbds/SKILL.md: update to write @fgadoc:jtbd annotations into files/model.fga; fix indentation note (4 spaces not 12) and ripgrep command (per copilot-pull-request-reviewer[bot]) - PERMISSIONS.md: update generated-intro source path to model.fga - docs/openfga.md: clarify that CI verifies co-editing of both files, not that version numbers were incremented — version bump is a social contract (per copilot-pull-request-reviewer[bot]) Resolves 4 review threads. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-1646 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
Contributor
Author
Review Feedback AddressedCommit: edccc5b Changes Made
No Change Needed
Threads Resolved5 of 5 unresolved threads addressed in this iteration. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.agents/skills/populate-jtbds/SKILL.md:
- Around line 280-284: The fenced code block that begins with lines containing
"# `@fgadoc`:jtbd" and "define <relation>:" is missing a language identifier and
triggers MD040; fix it by adding a language tag (e.g., text) after the opening
triple backticks so the block becomes ```text and retains the same contents,
ensuring the markdown linter no longer flags it.
🪄 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: 0ba49f62-213a-45da-9b30-077e0456f4f4
📒 Files selected for processing (5)
.agents/skills/populate-jtbds/SKILL.md.agents/skills/render-permissions/SKILL.md.github/workflows/fga-model-check.ymlPERMISSIONS.mddocs/openfga.md
✅ Files skipped from review due to trivial changes (1)
- PERMISSIONS.md
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/fga-model-check.yml
- docs/openfga.md
🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-1646 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Restore the yaml code fence indicator and the explicit whitespace in the ripgrep outlier-detection pattern that were incorrectly removed in the previous review commit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-1646 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
Address review comments from copilot-pull-request-reviewer[bot]: - .github/workflows/fga-model-check.yml: fix strip() to use sed to remove inline trailing comments (e.g. "# Parent relationship") so that inline-comment-only edits are correctly skipped (per copilot-pull-request-reviewer[bot]) - .agents/skills/populate-jtbds/SKILL.md: fix ripgrep outlier pattern to whitelist both 4-space relation-level lines and 0-space type-level @fgadoc: annotations; update Step 8 verify to note PERMISSIONS.md should also be regenerated after JTBD edits (per copilot-pull-request-reviewer[bot]) - docs/openfga.md: add step 3 to regenerate PERMISSIONS.md via the render-permissions skill after editing model.fga (per copilot-pull-request-reviewer[bot]) - PR description: remove overstatement about syntax validation and strip portability checks (per copilot-pull-request-reviewer[bot]) Resolves 5 review threads. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-1646 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
Contributor
Author
Review Feedback Addressed (Round 2)Commit: 7ba2db3 Changes Made
Threads Resolved5 of 5 unresolved threads addressed in this iteration. |
Contributor
Author
Review Feedback Addressed (Round 2)Commit: 7ba2db3 Changes Made
Threads Resolved5 of 5 unresolved threads addressed in this iteration. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The previous sed pattern used [[:space:]]* (zero or more spaces)
which also stripped # in DSL type references like team#member.
Fix by splitting the removal into two passes:
1. grep removes full-line comments (^[[:space:]]*#)
2. sed removes trailing inline comments ([[:space:]]+#.*$)
using \+ (one or more spaces) so team#member is preserved.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Issue: LFXV2-1646
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
Pass github.base_ref through an environment variable instead of interpolating it directly into the shell script, preventing a crafted branch name from injecting shell commands. Co-authored-by: Copilot (claude-sonnet-4.6) <223556219+Copilot@users.noreply.github.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fga-model-check: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
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
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
charts/lfx-platform/files/model.fgafile.Files.Getwithindent 8to preserve the YAML block scalarfga-model-check) that detects model changes and requires a corresponding edit tomodel.yaml(version bump is a social contract)🤖 Generated with Claude Code
Issue: LFXV2-1646
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Signed-off-by: Trevor Bramwell tbramwell@linuxfoundation.org