feat(core): optional namespace in actions commands and re-enable actions/RR tests#775
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a namespace requirement for actions commands in the CLI, enhancing the uniqueness and organization of actions. It also re-enables previously disabled E2E tests and updates the documentation to reflect these changes. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
elizabethhealy
changed the title
elizabethhealy
changed the title
![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 correctly adds the required namespace flag to all CLI actions commands and updates the underlying handlers and documentation. The E2E tests are also re-enabled and updated accordingly. My review includes a few suggestions to reduce code duplication in both the CLI command setup and the request handlers. I also found a potential bug in the UpdateAction handler where the new namespace parameter is not being used in the update request, which should be addressed.
There was a problem hiding this comment.
Code Review
This pull request introduces namespace support for policy actions by adding a required --namespace flag to all otdfctl policy actions CLI commands (get, list, create, update, delete). The changes include updating command definitions, modifying handler functions to accept and process the namespace (distinguishing between ID and FQN), and updating documentation and end-to-end tests. Review comments highlight opportunities to improve maintainability by extracting helper functions to reduce code duplication for both adding the namespace flag to CLI commands and for parsing the namespace string (UUID vs FQN) in the handler functions.
X-Test Results✅ go-main |
…thub.com/opentdf/otdfctl into dspx-2540-add-namespaced-actions-support
|
/gemini summary |
Summary of ChangesThis pull request introduces a fundamental change to how 'actions' are managed by enforcing a mandatory namespace requirement for all related CLI commands. This transition from globally unique actions to namespace-scoped actions significantly improves organization and prevents potential naming collisions. The changes also include the re-enabling and updating of end-to-end tests to validate the new behavior, alongside comprehensive documentation updates to guide users through the revised command structure. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
|
X-Test Failure Report✅ java-main |
X-Test Results✅ go-main |
X-Test Results✅ go-v0.9.0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
e2e/actions.bats (1)
93-93:⚠️ Potential issue | 🟡 MinorUse
run_otdfctl_action ... --jsonfor these ID lookups.These raw
$(./otdfctl ... | jq ...)captures bypass the helper and make the failing lookup much harder to localize when one of these commands breaks. Please capture the JSON viarun_otdfctl_action,assert_success, and then parse.idfrom$output.💡 Example
- UPDATE_ACTION_ID=$(./otdfctl policy actions get --name update --namespace "$ACTION_NAMESPACE" --json $HOST $WITH_CREDS | jq -r '.id') + run_otdfctl_action get --name update --namespace "$ACTION_NAMESPACE" --json + assert_success + UPDATE_ACTION_ID=$(echo "$output" | jq -er '.id')Run the following script to confirm the helper exists and these lookups still bypass it:
#!/bin/bash rg -n -C1 -F 'run_otdfctl_action ()' e2e/actions.bats rg -n -C1 -F '=$(./otdfctl policy actions' e2e/actions.batsExpected: the second search should return the remaining direct
policy actionslookups at the lines called out above.Also applies to: 156-156, 190-190, 196-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/actions.bats` at line 93, Replace the raw capture of the otdfctl JSON with the test helper: call run_otdfctl_action to run "./otdfctl policy actions get --name update --namespace \"$ACTION_NAMESPACE\" --json $HOST $WITH_CREDS", then call assert_success to ensure it succeeded and parse the ID from the helper's $output (e.g. UPDATE_ACTION_ID=$(echo "$output" | jq -r '.id')). Do the same replacement for the other direct captures flagged (the other policy actions get invocations) so all JSON lookups use run_otdfctl_action + assert_success + parsing from $output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/actions.bats`:
- Around line 60-63: Re-enable the e2e tests that assert failure when
--namespace is omitted and remove the passing list-without-namespace test by
making the namespace flag required for actions create and list; specifically
update the flag registration in cmd/policy/actions.go (the code that defines the
namespace flag for the "create" and "list" commands) to mark it required (or
return an error if missing) so the reinstated tests in e2e/actions.bats fail
when --namespace is not provided, then un-comment the failure-case in the e2e
test and delete/modify the passing list test that omits --namespace.
In `@e2e/registered-resources.bats`:
- Around line 25-28: The test currently resolves the "read" action globally and
may pick the wrong ID; update the lookup that sets READ_ACTION_ID to resolve
within the test namespace by passing the namespace flag (use the existing NS_ID)
to the otdfctl call that uses READ_ACTION_NAME and produces READ_ACTION_ID so
the command queries the namespace-scoped action; touch the
READ_ACTION_NAME/READ_ACTION_ID assignment (the otdfctl invocation that pipes to
jq -r '.id') to include the namespace flag so the test fetches the namespaced
action ID.
---
Duplicate comments:
In `@e2e/actions.bats`:
- Line 93: Replace the raw capture of the otdfctl JSON with the test helper:
call run_otdfctl_action to run "./otdfctl policy actions get --name update
--namespace \"$ACTION_NAMESPACE\" --json $HOST $WITH_CREDS", then call
assert_success to ensure it succeeded and parse the ID from the helper's $output
(e.g. UPDATE_ACTION_ID=$(echo "$output" | jq -r '.id')). Do the same replacement
for the other direct captures flagged (the other policy actions get invocations)
so all JSON lookups use run_otdfctl_action + assert_success + parsing from
$output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce270ff7-7295-41a0-83d2-5807991ff889
📒 Files selected for processing (3)
e2e/actions.batse2e/obligations.batse2e/registered-resources.bats
There was a problem hiding this comment.
♻️ Duplicate comments (3)
e2e/actions.bats (3)
33-44:⚠️ Potential issue | 🟡 MinorMake these cleanup paths fail closed.
The namespaced create path still scrapes the rendered table to find the ID, and the cleanup deletes here ignore
runfailure. If either step breaks, the test can leave actions behind and poison later runs. Create with--json, parse.id, and assert the delete succeeded.💡 Example pattern
- run_otdfctl_action create --name test_action_create_namespaced --namespace "$ACTION_NAMESPACE" - assert_output --partial "SUCCESS" - assert_line --regexp "Name.*test_action_create_namespaced" - assert_line --regexp "Namespace.*$ACTION_NAMESPACE" - assert_output --partial "Id" - assert_output --partial "Created At" - assert_line --partial "Updated At" + run_otdfctl_action create --name test_action_create_namespaced --namespace "$ACTION_NAMESPACE" --json + assert_success + assert_equal "$(echo "$output" | jq -r '.name')" "test_action_create_namespaced" + assert_equal "$(echo "$output" | jq -r '.namespace.fqn')" "$ACTION_NAMESPACE" - created_id=$(echo "$output" | grep Id | awk -F'│' '{print $3}' | xargs) - run_otdfctl_action delete --id $created_id --force + created_id=$(echo "$output" | jq -er '.id') + [ -n "$created_id" ] + run_otdfctl_action delete --id "$created_id" --force + assert_successVerification: inspect the changed regions for table scraping and unasserted cleanup deletes. Expected: the script reports the table parse on Line 43 and cleanup deletes in these changed blocks.
#!/bin/bash python - <<'PY' from pathlib import Path lines = Path("e2e/actions.bats").read_text().splitlines() for lineno, line in enumerate(lines, 1): if 33 <= lineno <= 44 or 89 <= lineno <= 100: if "grep Id | awk" in line or "run_otdfctl_action delete --id" in line: print(f"{lineno}: {line}") PYAlso applies to: 89-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/actions.bats` around lines 33 - 44, The namespaced create path uses table scraping to get the ID and the delete run ignores failures; change the create invocation (run_otdfctl_action create --name test_action_create_namespaced --namespace ...) to use --json output, extract the created id from the JSON (parse the .id field) instead of grepping the table, and then call run_otdfctl_action delete --id <parsed_id> --force while asserting the delete command succeeded (fail the test if the delete run exits non‑zero or its output is not the expected success marker). Ensure you update the ID variable assignment to use the JSON parser and replace the unguarded delete invocation so failures are asserted.
215-218:⚠️ Potential issue | 🟠 MajorThis negative delete test can pass for the wrong reason.
If the lookup on Line 215 returns
null/empty,delete --id "$STANDARD_ACTION"still fails andassert_failurepasses without ever exercising the "cannot delete standard action" path. Resolve the ID viarun_otdfctl_action get --json, assert that lookup succeeded, and assert the returned ID is non-empty before the delete.💡 Safer setup for the negative case
- STANDARD_ACTION=$(./otdfctl policy actions get --name update --namespace "$ACTION_NAMESPACE" $HOST $WITH_CREDS --json | jq -r '.id') + run_otdfctl_action get --name update --namespace "$ACTION_NAMESPACE" --json + assert_success + STANDARD_ACTION=$(echo "$output" | jq -er '.id') + [ -n "$STANDARD_ACTION" ] run_otdfctl_action delete --id "$STANDARD_ACTION" --force assert_failureVerification: inspect the negative delete case. Expected: the setup currently captures the ID via
jq -rand only asserts that delete failed.#!/bin/bash python - <<'PY' from pathlib import Path lines = Path("e2e/actions.bats").read_text().splitlines() for i in range(214, 219): print(f"{i}: {lines[i-1]}") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/actions.bats` around lines 215 - 218, The negative delete test can pass spuriously when STANDARD_ACTION is empty; change the setup to call run_otdfctl_action get --json for the "update" action (instead of using otdfctl policy actions get | jq -r) capture and parse the returned JSON to extract the id, then assert the get call succeeded and that STANDARD_ACTION (the extracted id variable) is non-empty before calling run_otdfctl_action delete --id "$STANDARD_ACTION" --force; update the test to fail early if the lookup fails so the delete assertion truly verifies the "cannot delete standard action" path.
11-11:⚠️ Potential issue | 🟡 MinorFail fast when capturing IDs from
otdfctl.These bare
$(./otdfctl ... | jq -r '.id')assignments can keep going with anull/empty ID and hide the real lookup/create failure behind thejqstep. In the tests, preferrun_otdfctl_action ... --json+assert_success; insetup_file, at least usejq -er '.id'withpipefailso fixture setup aborts immediately.Verification: inspect the remaining direct
./otdfctl ... | jq -r '.id'captures. Expected: Lines 11, 111, 181, and 221 are reported.#!/bin/bash python - <<'PY' from pathlib import Path path = Path("e2e/actions.bats") for lineno, line in enumerate(path.read_text().splitlines(), 1): if "./otdfctl" in line and "jq -r '.id'" in line: print(f"{lineno}: {line}") PYAlso applies to: 111-111, 181-181, 221-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/actions.bats` at line 11, The setup assigns NS_ID using a subshell that pipes otfdctl into jq -r '.id', which will silently produce empty/null IDs on upstream failures; change these captures (e.g. the NS_ID export line and the other direct captures at the reported locations) to fail-fast: in test helpers use the existing run_otdfctl_action ... --json + assert_success pattern, and in setup_file enable pipefail and use jq -er '.id' so jq fails on missing/null id, ensuring the script aborts immediately on otfdctl failure rather than continuing with a null ID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e/actions.bats`:
- Around line 33-44: The namespaced create path uses table scraping to get the
ID and the delete run ignores failures; change the create invocation
(run_otdfctl_action create --name test_action_create_namespaced --namespace ...)
to use --json output, extract the created id from the JSON (parse the .id field)
instead of grepping the table, and then call run_otdfctl_action delete --id
<parsed_id> --force while asserting the delete command succeeded (fail the test
if the delete run exits non‑zero or its output is not the expected success
marker). Ensure you update the ID variable assignment to use the JSON parser and
replace the unguarded delete invocation so failures are asserted.
- Around line 215-218: The negative delete test can pass spuriously when
STANDARD_ACTION is empty; change the setup to call run_otdfctl_action get --json
for the "update" action (instead of using otdfctl policy actions get | jq -r)
capture and parse the returned JSON to extract the id, then assert the get call
succeeded and that STANDARD_ACTION (the extracted id variable) is non-empty
before calling run_otdfctl_action delete --id "$STANDARD_ACTION" --force; update
the test to fail early if the lookup fails so the delete assertion truly
verifies the "cannot delete standard action" path.
- Line 11: The setup assigns NS_ID using a subshell that pipes otfdctl into jq
-r '.id', which will silently produce empty/null IDs on upstream failures;
change these captures (e.g. the NS_ID export line and the other direct captures
at the reported locations) to fail-fast: in test helpers use the existing
run_otdfctl_action ... --json + assert_success pattern, and in setup_file enable
pipefail and use jq -er '.id' so jq fails on missing/null id, ensuring the
script aborts immediately on otfdctl failure rather than continuing with a null
ID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef8f4795-a16f-4ccb-af7c-3e0599aa7cf8
📒 Files selected for processing (8)
cmd/policy/actions.goe2e/actions.batse2e/obligations.batse2e/registered-resources.batspkg/handlers/actions.gopkg/handlers/namespaces.gopkg/handlers/obligations.gopkg/handlers/registeredResources.go
✅ Files skipped from review due to trivial changes (1)
- pkg/handlers/namespaces.go
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/registered-resources.bats
- pkg/handlers/actions.go
- cmd/policy/actions.go
X-Test Failure Report |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
e2e/actions.bats (1)
43-45:⚠️ Potential issue | 🟡 MinorAssert cleanup deletions to avoid false-green tests.
Line 44, Line 57, Line 100, Line 190, Line 191, and Line 225 run cleanup deletes without validating success. If cleanup fails, later tests can be polluted by leftover actions.
Suggested fix
run_otdfctl_action delete --id "$created_id" --force + assert_success run_otdfctl_action delete --id "$created_id" --force + assert_success run_otdfctl_action delete --id "$conflict_action_id" --force + assert_success run_otdfctl_action delete --id "$created_id" --force + assert_success run_otdfctl_action delete --id "$created_id_2" --force + assert_success run_otdfctl_action delete --id "$ACTION_TO_UPDATE" --force + assert_successAlso applies to: 56-57, 99-101, 190-191, 225-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/actions.bats` around lines 43 - 45, The cleanup delete commands (the lines that set created_id and call run_otdfctl_action delete --id $created_id --force) must be asserted so failures don't leave leftover resources: capture the delete command's exit status/output and add an assertion that it succeeded (e.g., check $status is 0 or that the delete output contains an expected success message), and optionally verify the resource is gone by listing/getting the id and asserting it is not present; apply this change to every cleanup delete invocation that uses created_id or run_otdfctl_action delete (the occurrences around lines 44, 56-57, 99-101, 190-191, 225-226) so each delete is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e/actions.bats`:
- Around line 43-45: The cleanup delete commands (the lines that set created_id
and call run_otdfctl_action delete --id $created_id --force) must be asserted so
failures don't leave leftover resources: capture the delete command's exit
status/output and add an assertion that it succeeded (e.g., check $status is 0
or that the delete output contains an expected success message), and optionally
verify the resource is gone by listing/getting the id and asserting it is not
present; apply this change to every cleanup delete invocation that uses
created_id or run_otdfctl_action delete (the occurrences around lines 44, 56-57,
99-101, 190-191, 225-226) so each delete is validated.
X-Test Failure Report |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/policy/actions.go (1)
187-194: Optional cleanup: cache the namespace doc flag once in helper.This reduces repeated lookups and makes the helper easier to scan.
♻️ Suggested refactor
func injectNamespaceFlag(doc *man.Doc) { + nsFlag := doc.GetDocFlag("namespace") doc.Flags().StringP( - doc.GetDocFlag("namespace").Name, - doc.GetDocFlag("namespace").Shorthand, - doc.GetDocFlag("namespace").Default, - doc.GetDocFlag("namespace").Description, + nsFlag.Name, + nsFlag.Shorthand, + nsFlag.Default, + nsFlag.Description, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/policy/actions.go` around lines 187 - 194, Cache the result of doc.GetDocFlag("namespace") into a local variable inside injectNamespaceFlag and use that variable's Name, Shorthand, Default, and Description fields when calling doc.Flags().StringP; this avoids calling doc.GetDocFlag("namespace") multiple times and makes injectNamespaceFlag easier to read (reference: injectNamespaceFlag and doc.GetDocFlag("namespace")).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/policy/actions.go`:
- Around line 187-194: Cache the result of doc.GetDocFlag("namespace") into a
local variable inside injectNamespaceFlag and use that variable's Name,
Shorthand, Default, and Description fields when calling doc.Flags().StringP;
this avoids calling doc.GetDocFlag("namespace") multiple times and makes
injectNamespaceFlag easier to read (reference: injectNamespaceFlag and
doc.GetDocFlag("namespace")).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03a645c4-ea9b-4708-a71a-5060522eada6
📒 Files selected for processing (1)
cmd/policy/actions.go
🤖 I have created a release *beep* *boop* --- ## [0.30.0](v0.29.0...v0.30.0) (2026-03-31) ### Features * **core:** Add optional namespace flag for subject mappings and condtion sets ([#779](#779)) ([9e849c4](9e849c4)) * **core:** add scope support for client creds ([#752](#752)) ([9ca9e43](9ca9e43)) * **core:** migrate registered resources ([#772](#772)) ([2b49a7d](2b49a7d)) * **core:** optional namespace in actions commands and re-enable actions/RR tests ([#775](#775)) ([29a2eb1](29a2eb1)) * **core:** support namespaced registered resources ([#767](#767)) ([4d786b5](4d786b5)) ### Bug Fixes * **ci:** Temporarily skip namespaced-actions impacted BATS cases ([#773](#773)) ([633728a](633728a)) * **core:** bump toolchain to go 1.24.13 ([#747](#747)) ([6804b93](6804b93)) * **core:** disable RR E2E tests ([#768](#768)) ([0821b8c](0821b8c)) * **core:** make namespacing registered resources optional ([#785](#785)) ([8e6eb31](8e6eb31)) * **core:** refactor `ListAttributesValues` to use `Get` ([#769](#769)) ([a82f7b7](a82f7b7)) * **core:** unsafe update result output values order ([#759](#759)) ([baeba0f](baeba0f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
2 participants
This pull request introduces a namespace flag for actions commands in the CLI, enhancing the uniqueness and organization of actions. It also re-enables previously disabled E2E tests and updates the documentation to reflect these changes.
Highlights
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores