Skip to content

Commit c11680b

Browse files
c-r33dc-r33d
authored
feat(cli): Add interactive review for prune plans (#3421)
## Branch Summary Branch: prune-plan-review ### What Changed - Added interactive review support for prune plans. - Renamed migration interactive review files for clarity: - interactive_review.go -> migration_review.go - interactive_review_test.go -> migration_review_test.go - Added prune review flow in prune_review.go. - Added shared policy formatting helpers in policy_formatting.go. - Added prune plan item interfaces and helper methods in prune_plan.go. - Added TargetRef.String() and PruneStatusReason.String() formatting. - Updated prune planner behavior to use prune status reason messages consistently. - Moved shared formatting helpers out of summary.go: - appendDetails - strconvQuote ### Interactive Prune Review Behavior The prune review flow now prompts for unresolved prune items across: - actions - subject condition sets - subject mappings - registered resources - obligation triggers For each unresolved item, the user can: - delete the source object - skip and leave it unresolved - abort the review Choosing delete now clears the prune reason. ### Formatting Added reusable formatting for policy-related prompt details: - action name lists - registered resource values and action bindings - obligation labels - request contexts - target refs Target refs now render as structured fields, including empty values: id="target-1" (namespace="https://example.com") id="" (namespace="") <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Added interactive review functionality for namespace policy prune plans, enabling users to review, approve, skip, or abort individual migration items with detailed context and reasoning. * **Refactor** * Improved formatting and display of policy structures, including better human-readable summaries for actions, resources, and obligations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent b8abf17 commit c11680b

11 files changed

Lines changed: 1147 additions & 29 deletions

otdfctl/migrations/namespacedpolicy/interactive_review.go renamed to otdfctl/migrations/namespacedpolicy/migration_review.go

File renamed without changes.

otdfctl/migrations/namespacedpolicy/interactive_review_test.go renamed to otdfctl/migrations/namespacedpolicy/migration_review_test.go

File renamed without changes.

‎otdfctl/migrations/namespacedpolicy/policy_formatting.go‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
package namespacedpolicy
2+
3+
import (
4+
"fmt"
5+
"strings"
6+
7+
"github.com/opentdf/platform/protocol/go/policy"
8+
)
9+
10+
func plainPolicyActionNamesSummary(actions []*policy.Action) string {
11+
names := make([]string, 0, len(actions))
12+
seen := make(map[string]struct{}, len(actions))
13+
for _, action := range actions {
14+
if action == nil {
15+
continue
16+
}
17+
name := actionLabel(action)
18+
if strings.TrimSpace(name) == "" || name == unknownLabel {
19+
continue
20+
}
21+
if _, ok := seen[name]; ok {
22+
continue
23+
}
24+
seen[name] = struct{}{}
25+
names = append(names, strconvQuote(name))
26+
}
27+
return plainListSummary(names)
28+
}
29+
30+
func plainRegisteredResourceSourceSummary(resource *policy.RegisteredResource) string {
31+
return appendDetails(
32+
"values="+plainRegisteredResourceValuesSummary(resource),
33+
"action_bindings="+plainRegisteredResourceActionAttributeValuesSummary(resource),
34+
)
35+
}
36+
37+
func appendDetails(line string, details ...string) string {
38+
filtered := make([]string, 0, len(details))
39+
for _, detail := range details {
40+
if strings.TrimSpace(detail) != "" {
41+
filtered = append(filtered, detail)
42+
}
43+
}
44+
if len(filtered) == 0 {
45+
return line
46+
}
47+
return fmt.Sprintf("%s (%s)", line, strings.Join(filtered, ", "))
48+
}
49+
50+
func strconvQuote(value string) string {
51+
return fmt.Sprintf("%q", value)
52+
}
53+
54+
func plainRegisteredResourceValuesSummary(resource *policy.RegisteredResource) string {
55+
values := make([]string, 0, len(resource.GetValues()))
56+
seen := make(map[string]struct{}, len(resource.GetValues()))
57+
for _, value := range resource.GetValues() {
58+
if value == nil {
59+
continue
60+
}
61+
label := strings.TrimSpace(value.GetValue())
62+
if label == "" {
63+
label = strings.TrimSpace(value.GetId())
64+
}
65+
if label == "" {
66+
continue
67+
}
68+
if _, ok := seen[label]; ok {
69+
continue
70+
}
71+
seen[label] = struct{}{}
72+
values = append(values, strconvQuote(label))
73+
}
74+
return plainListSummary(values)
75+
}
76+
77+
func plainRegisteredResourceActionAttributeValuesSummary(resource *policy.RegisteredResource) string {
78+
bindings := make([]string, 0)
79+
seen := make(map[string]struct{})
80+
for _, value := range resource.GetValues() {
81+
if value == nil {
82+
continue
83+
}
84+
for _, binding := range value.GetActionAttributeValues() {
85+
if binding == nil {
86+
continue
87+
}
88+
label := fmt.Sprintf("%s -> %s", strconvQuote(actionLabel(binding.GetAction())), valueFQN(binding.GetAttributeValue()))
89+
if _, ok := seen[label]; ok {
90+
continue
91+
}
92+
seen[label] = struct{}{}
93+
bindings = append(bindings, label)
94+
}
95+
}
96+
return plainListSummary(bindings)
97+
}
98+
99+
func obligationLabel(obligation *policy.Obligation) string {
100+
if obligation == nil {
101+
return noneLabel
102+
}
103+
if fqn := strings.TrimSpace(obligation.GetFqn()); fqn != "" {
104+
return fqn
105+
}
106+
if name := strings.TrimSpace(obligation.GetName()); name != "" {
107+
return name
108+
}
109+
if id := strings.TrimSpace(obligation.GetId()); id != "" {
110+
return id
111+
}
112+
return noneLabel
113+
}
114+
115+
func plainRequestContextsSummary(contexts []*policy.RequestContext) string {
116+
clientIDs := make([]string, 0, len(contexts))
117+
seen := make(map[string]struct{}, len(contexts))
118+
for _, requestContext := range contexts {
119+
clientID := strings.TrimSpace(requestContext.GetPep().GetClientId())
120+
if clientID == "" {
121+
continue
122+
}
123+
if _, ok := seen[clientID]; ok {
124+
continue
125+
}
126+
seen[clientID] = struct{}{}
127+
clientIDs = append(clientIDs, "client_id="+strconvQuote(clientID))
128+
}
129+
return plainListSummary(clientIDs)
130+
}
131+
132+
func plainListSummary(items []string) string {
133+
if len(items) == 0 {
134+
return noneLabel
135+
}
136+
return strings.Join(items, ", ")
137+
}

‎otdfctl/migrations/namespacedpolicy/policy_formatting_test.go‎

Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,176 @@
1+
package namespacedpolicy
2+
3+
import (
4+
"testing"
5+
6+
"github.com/opentdf/platform/protocol/go/policy"
7+
"github.com/stretchr/testify/assert"
8+
)
9+
10+
func TestPlainPolicyActionNamesSummary(t *testing.T) {
11+
t.Parallel()
12+
13+
tests := []struct {
14+
name string
15+
actions []*policy.Action
16+
want string
17+
}{
18+
{name: "empty", want: noneLabel},
19+
{
20+
name: "skips nil and deduplicates labels",
21+
actions: []*policy.Action{
22+
nil,
23+
{Id: "action-1", Name: "read"},
24+
{Id: "action-2", Name: "write"},
25+
{Id: "action-3", Name: "read"},
26+
},
27+
want: `"read", "write"`,
28+
},
29+
{
30+
name: "falls back to id",
31+
actions: []*policy.Action{
32+
{Id: "action-1"},
33+
},
34+
want: `"action-1"`,
35+
},
36+
{
37+
name: "ignores empty labels",
38+
actions: []*policy.Action{
39+
{},
40+
},
41+
want: noneLabel,
42+
},
43+
}
44+
45+
for _, tc := range tests {
46+
t.Run(tc.name, func(t *testing.T) {
47+
t.Parallel()
48+
assert.Equal(t, tc.want, plainPolicyActionNamesSummary(tc.actions))
49+
})
50+
}
51+
}
52+
53+
func TestPlainRegisteredResourceSourceSummary(t *testing.T) {
54+
t.Parallel()
55+
56+
secretValue := testAttributeValue("https://example.com/attr/classification/value/secret", testNamespace("https://example.com"))
57+
58+
tests := []struct {
59+
name string
60+
resource *policy.RegisteredResource
61+
want string
62+
}{
63+
{name: "nil", want: "values=(none) (action_bindings=(none))"},
64+
{
65+
name: "empty values",
66+
resource: &policy.RegisteredResource{},
67+
want: "values=(none) (action_bindings=(none))",
68+
},
69+
{
70+
name: "deduplicates values and bindings",
71+
resource: testRegisteredResource(
72+
"resource-1",
73+
"dataset",
74+
testRegisteredResourceValue("prod", testActionAttributeValue("action-1", "read", secretValue)),
75+
testRegisteredResourceValue("prod", testActionAttributeValue("action-1", "read", secretValue)),
76+
testRegisteredResourceValue("dev", testActionAttributeValue("action-2", "write", secretValue)),
77+
),
78+
want: `values="prod", "dev" (action_bindings="read" -> https://example.com/attr/classification/value/secret, "write" -> https://example.com/attr/classification/value/secret)`,
79+
},
80+
{
81+
name: "value falls back to id",
82+
resource: testRegisteredResource(
83+
"resource-1",
84+
"dataset",
85+
&policy.RegisteredResourceValue{Id: "value-1"},
86+
),
87+
want: `values="value-1" (action_bindings=(none))`,
88+
},
89+
}
90+
91+
for _, tc := range tests {
92+
t.Run(tc.name, func(t *testing.T) {
93+
t.Parallel()
94+
assert.Equal(t, tc.want, plainRegisteredResourceSourceSummary(tc.resource))
95+
})
96+
}
97+
}
98+
99+
func TestObligationLabel(t *testing.T) {
100+
t.Parallel()
101+
102+
tests := []struct {
103+
name string
104+
obligation *policy.Obligation
105+
want string
106+
}{
107+
{name: "nil", want: noneLabel},
108+
{
109+
name: "uses fqn first",
110+
obligation: &policy.Obligation{
111+
Id: "obligation-1",
112+
Name: "watermark",
113+
Fqn: "https://example.com/obl/watermark",
114+
},
115+
want: "https://example.com/obl/watermark",
116+
},
117+
{
118+
name: "falls back to name",
119+
obligation: &policy.Obligation{
120+
Id: "obligation-1",
121+
Name: "watermark",
122+
},
123+
want: "watermark",
124+
},
125+
{
126+
name: "falls back to id",
127+
obligation: &policy.Obligation{
128+
Id: "obligation-1",
129+
},
130+
want: "obligation-1",
131+
},
132+
{
133+
name: "empty",
134+
obligation: &policy.Obligation{},
135+
want: noneLabel,
136+
},
137+
}
138+
139+
for _, tc := range tests {
140+
t.Run(tc.name, func(t *testing.T) {
141+
t.Parallel()
142+
assert.Equal(t, tc.want, obligationLabel(tc.obligation))
143+
})
144+
}
145+
}
146+
147+
func TestPlainRequestContextsSummary(t *testing.T) {
148+
t.Parallel()
149+
150+
tests := []struct {
151+
name string
152+
contexts []*policy.RequestContext
153+
want string
154+
}{
155+
{name: "empty", want: noneLabel},
156+
{
157+
name: "skips empty and deduplicates client ids",
158+
contexts: []*policy.RequestContext{
159+
nil,
160+
{},
161+
{Pep: &policy.PolicyEnforcementPoint{}},
162+
{Pep: &policy.PolicyEnforcementPoint{ClientId: "tdf-client"}},
163+
{Pep: &policy.PolicyEnforcementPoint{ClientId: "tdf-client"}},
164+
{Pep: &policy.PolicyEnforcementPoint{ClientId: "admin-client"}},
165+
},
166+
want: `client_id="tdf-client", client_id="admin-client"`,
167+
},
168+
}
169+
170+
for _, tc := range tests {
171+
t.Run(tc.name, func(t *testing.T) {
172+
t.Parallel()
173+
assert.Equal(t, tc.want, plainRequestContextsSummary(tc.contexts))
174+
})
175+
}
176+
}

0 commit comments

Comments
 (0)