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.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 32106205339Coverage decreased (-0.02%) to 48.764%Details
Uncovered Changes
Coverage Regressions4 previously-covered lines in 2 files 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. |
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.