Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request outlines a migration plan for namespacing policy items such as actions, subject mappings, and registered resources using a graph-based, create-only approach. The documentation covers namespace determination rules, execution order, and a phased rollout strategy. Feedback was provided regarding inconsistencies in the description of the prune fallback mechanism, the need for better alignment of metadata update steps in the execution plan, and a request for clarification on the specific conditions that trigger an SCS namespace conflict blocker.
| otdfctl migrate prune registered-resources --from=migration.json --commit | ||
| ``` | ||
|
|
||
| Prune reads the migration manifest (JSON file emitted by `--output`) to know exactly which old unnamespaced items map to which new namespaced copies. If `--from` is not provided, prune falls back to re-deriving matches (by name for actions/RR, by content for SCS, by attribute-value-id+actions+SCS for SM). Items that were never migrated are left untouched and flagged in the output. |
There was a problem hiding this comment.
The description of the prune fallback mechanism here is inconsistent with the more robust approach described later in the document (line 236). This line suggests a heuristic re-derivation of matches, while line 236 states that the primary mechanism will be filtering for items with the wasMigrated=true label. The label-based approach is much safer and less error-prone. It would be best to update this section to consistently describe the label-based approach as the primary fallback.
| 3. If actions in scope: create actions via `handler.CreateAction(name, targetNS, nil)`. For reuse cases, skip creation and record existing action ID. | ||
| 4. If SCS in scope: create SCS clones via `handler.CreateSubjectConditionSet(subjectSets, metadata, targetNS)` | ||
| 5. If SM in scope: create new SM in target NS via `handler.CreateNewSubjectMapping(attrValID, rewrittenActions, newSCSID, nil, metadata, targetNS)`. Then update old SM metadata to add `wasMigrated=true`. Old SM is NOT deleted. | ||
| 6. If RR in scope: create new RR + values in target NS (using existing `commitRegisteredResourceMigration` logic, modified to skip the delete step). Then update old RR metadata to add `wasMigrated=true`. Old RR is NOT deleted. | ||
| 7. For actions and SCS: after creating namespaced copies, update old items' metadata to add `wasMigrated=true`. |
There was a problem hiding this comment.
The execution plan for actions (step 3) and SCS (step 4) doesn't mention updating the old item's metadata, while this is explicitly part of the steps for SM (step 5) and RR (step 6). Step 7 then adds this for actions and SCS.
For better clarity and consistency, I suggest incorporating the metadata update directly into steps 3 and 4, and then removing step 7. For example:
- Step 3: "... After creating the namespaced action, update the old action's metadata to add
wasMigrated=true." - Step 4: "... After creating the SCS clone, update the old SCS's metadata to add
wasMigrated=true."
This would make the execution flow for each resource type symmetrical.
| ### SM/SCS blockers | ||
| - `SM_NAMESPACE_UNDETERMINED` — attribute value has no parseable namespace FQN | ||
| - `SM_ACTION_NOT_IN_PLAN` — SM references action not covered by action plan | ||
| - `SCS_NAMESPACE_CONFLICT` — SCS clone target can't be determined |
There was a problem hiding this comment.
The condition for the SCS_NAMESPACE_CONFLICT blocker is unclear. The "Namespace Determination Rules" on line 28 state that if an SCS is referenced by SMs in multiple namespaces, it will be cloned into each namespace. This seems to handle the multi-namespace case. Could you please clarify under what specific circumstances an SCS_NAMESPACE_CONFLICT would occur? For example, does this happen if the referencing SMs themselves have an undetermined namespace?
1 participant
No description provided.