fix(reconcile): identify permissions by key instead of namespace and name - #1892
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 32 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report for CI Build 32239944274Coverage decreased (-0.003%) to 48.804%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions3 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
e363944 to
cf77ebc
Compare
|
Review: identify permissions by key (RFC 0001 Rule 1) Verified on this branch: The code achieves Rule 1 cleanly. The rebase onto the stack resolved the earlier overlap with #1889: this branch now sits on top of #1889 and inherits its tightened grammar (the verb error even carries #1889's wording). The shared permission-validation items (the CreatePermission API not validating namespace grammar, and the combined slug-length bound) are tracked in #1889 so they are not duplicated here. Fixing them there fixes them for this PR too. Verdict: changes needed. The code is approvable; the blocker is a doc file this PR claims to have migrated but did not. Findings:
|
e70b648 to
345417c
Compare
cf77ebc to
6bdbd09
Compare
|
Follow-up on the findings.
Also, the stack was rebased so this PR now sits on top of #1889 and inherits its tightened grammar and the new slug-length bound, so those apply to the key form too. |
|
Re-review (head 6bdbd09) Build, tests, and lint are all green. Both code findings are resolved.
One doc item remains, so this is not fully closed. The bullet list "A few rules the reconcile flow enforces for permissions" at
A reader who builds a file from that prose, rather than copying the example above it, would still write Verdict: approve with nits, once the bullet list is updated. |
6bdbd09 to
04c4f1a
Compare
|
Doc consistency (not inline, since the file is not in this diff): this PR updates |
4648a87 to
ea6d8fd
Compare
04c4f1a to
8c2fbdc
Compare
|
Fixed. |
ea6d8fd to
b720e4a
Compare
8c2fbdc to
4321423
Compare
b720e4a to
688963e
Compare
4321423 to
422103b
Compare
688963e to
331a656
Compare
…so a legacy row cannot wedge the plan
… tighten the grammar wording
6c986cf to
de5da44
Compare
What
The Permission reconcile kind now uses one
keyfield instead of separatenamespaceandnamefields. The key is theservice.resource.verbvalue that the API already returns and stores.Before:
After:
Why
The API's permission identity is the key. The reconciler used to read the deprecated
namespaceandnameresponse fields and rebuild the key at apply time. Reading the key straight from the API is simpler, drops the deprecated-field use, and makes the file match what the server reports.Changes
PermissionSpecis now{ key, delete }. The diff, validate, and export paths all key off the permission key.fetchCurrentreadsGetKey()and parses it into namespace and verb only to skip base-schema permissions. A key that does not parse intoservice.resource.verb(a base or system permission, or an older server that does not set the key) is skipped, not fatal.keyentries sorted by key, so a re-reconcile of an exported file plans nothing.Breaking change
This is a hard switch. A permission file that still uses
namespaceandnamenow fails the plan with an unknown-field error instead of silently reconciling nothing. Any pixxelhq/configs permission file must move to the key form in a separate configs change.Stacking
This PR is the top of the reconcile-audit stack: #1888 -> #1889 -> #1890 -> #1891 -> this. It builds on #1889, which tightens the permission grammar. Review and merge in stack order.