Skip to content

Conversation

@elizabethhealy
Copy link
Member

  • command for resource mapping groups
  • ability to specify a group on resource mapping creation

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @elizabethhealy, 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!

Summary of Changes

Hello! Gemini here, providing a summary of this pull request. This PR introduces a new set of CLI commands for managing 'Resource Mapping Groups'. Resource Mapping Groups allow users to logically group related resource mappings. The changes include adding full CRUD (Create, Get, List, Update, Delete) functionality for these groups via the policy resource-mapping-groups command. Additionally, the existing policy resource-mappings create and update commands have been enhanced to allow associating a resource mapping with a specific group via a new --group-id flag. The output for resource mapping commands has also been updated to display the associated group information.

Highlights

  • New Command Group: Adds a new top-level command policy resource-mapping-groups to the CLI.
  • Resource Mapping Group Management: Implements create, get, list, update, and delete subcommands for managing resource mapping groups.
  • Associate Mappings with Groups: Adds an optional --group-id flag to policy resource-mappings create and update commands, allowing users to assign a resource mapping to a group upon creation or update.
  • Enhanced Output: Updates the output of policy resource-mappings commands (create, get, list, update, delete) to include the associated Resource Mapping Group ID and Name.
  • Documentation: Adds comprehensive man page documentation for the new resource-mapping-groups commands and updates documentation for the modified resource-mappings commands.

Changelog

Click here to see the changelog
  • cmd/policy-resourceMappingGroups.go
    • New file implementing the CLI commands for resource mapping groups (create, get, list, update, delete).
    • Includes functions to handle command execution, flag parsing, and interaction with the SDK handlers.
    • Sets up Cobra commands and integrates them under the policy command.
  • cmd/policy-resourceMappings.go
    • Added an optional group-id flag to the create and update commands.
    • Modified the policy_createResourceMapping function to accept and pass the optional group ID to the handler (lines 25, 31).
    • Modified the policy_updateResourceMapping function to accept and pass the optional group ID to the handler (lines 127, 131).
    • Updated the tabular and table output functions (policy_createResourceMapping, policy_getResourceMapping, policy_listResourceMappings, policy_updateResourceMapping, policy_deleteResourceMapping) to include Group ID and Group Name (lines 40-41, 66-67, 94-95, 107-108, 140-141, 174-175).
    • Added flag definitions for --group-id in the init function for create and update commands (lines 196-200, 236-240).
  • docs/man/policy/resource-mapping-groups/_index.md
    • New file providing an overview and aliases for the resource-mapping-groups command.
  • docs/man/policy/resource-mapping-groups/create.md
    • New file documenting the create subcommand for resource mapping groups.
  • docs/man/policy/resource-mapping-groups/delete.md
    • New file documenting the delete subcommand for resource mapping groups.
  • docs/man/policy/resource-mapping-groups/get.md
    • New file documenting the get subcommand for resource mapping groups.
  • docs/man/policy/resource-mapping-groups/list.md
    • New file documenting the list subcommand for resource mapping groups.
  • docs/man/policy/resource-mapping-groups/update.md
    • New file documenting the update subcommand for resource mapping groups.
  • docs/man/policy/resource-mappings/create.md
    • Added documentation for the new --group-id flag (lines 16-18).
  • docs/man/policy/resource-mappings/update.md
    • Added documentation for the new --group-id flag (lines 17-19).
    • Updated the description to mention changing the group (line 29).
  • pkg/handlers/resourceMappingGroups.go
    • New file implementing handler functions for resource mapping group operations (CreateResourceMappingGroup, GetResourceMappingGroup, ListResourceMappingGroups, UpdateResourceMappingGroup, DeleteResourceMappingGroup).
    • These functions interact with the underlying SDK.
  • pkg/handlers/resourceMappings.go
    • Modified the CreateResourceMapping function signature and implementation to accept and use an optional grpId (lines 18, 21).
    • Modified the UpdateResourceMapping function signature and implementation to accept and use an optional grpId (lines 59, 64).
Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and 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 to provide feedback.

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.


A new command takes flight,
Grouping mappings, shining bright.
Policy now neat,
A developer's treat,
All managed with CLI might.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a significant new feature: resource mapping groups, along with the necessary CLI commands (create, get, list, update, delete) and updates to existing resource mapping commands to associate them with these groups. The corresponding documentation has also been added.

The core handler logic for interacting with the SDK seems straightforward. However, there are several areas in the CLI command implementations and documentation that need attention, including incorrect error messages, display of incorrect data, a critical bug in command wiring, and some documentation inaccuracies. Addressing these will greatly improve the quality and usability of this new functionality.

Summary of Findings

  • Critical Command Wiring Error: In cmd/policy-resourceMappingGroups.go, the list subcommand for resource mapping groups is incorrectly wired to policy_listResourceMappings instead of policy_listResourceMappingGroups. This needs immediate correction.
  • Incorrect Data Display in CLI Output: Multiple CLI commands (create, get, update, delete) for resource mapping groups incorrectly display the group's own ID as the 'Namespace ID' in the tabular output. This should be corrected to display resourceMappingGroup.GetNamespaceId().
  • Misleading Error Messages: Several error messages in cmd/policy-resourceMappingGroups.go refer to "resource mapping" instead of "resource mapping group", which could confuse users.
  • Documentation Inaccuracies: The documentation for resource mapping groups contains some errors: incorrect command description for create, an incorrect example for list, and an incorrect title for update.
  • Pending Verification for Update Behavior: A TODO comment in pkg/handlers/resourceMappingGroups.go indicates that the update behavior for resource mapping groups needs verification. This should be addressed to ensure correctness.
  • Minor Documentation Typos: There are minor typos (e.g., "namesapce" instead of "namespace") in the documentation files docs/man/policy/resource-mapping-groups/create.md (line 11) and docs/man/policy/resource-mapping-groups/update.md (line 12). (Not commented inline due to review settings).
  • Missing Newline in Markdown File: The file docs/man/policy/resource-mapping-groups/_index.md is missing a newline character at the end of the file. (Not commented inline due to review settings).

Merge Readiness

This pull request introduces valuable functionality for managing resource mapping groups. However, due to the presence of a critical issue (incorrect command wiring) and several high-severity issues (incorrect data display), along with medium-severity issues related to error messages, documentation, and a pending TODO for behavior verification, I recommend that these changes not be merged until these issues are addressed. Resolving these points will significantly improve the reliability and user experience of the new CLI commands. As a reviewer, I am not authorized to approve pull requests; please ensure further review and approval from other maintainers after the suggested changes are made.

@jakedoublev jakedoublev requested a review from Copilot May 29, 2025 21:51
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds support for grouping resource mappings by introducing a group-id parameter on creation and updates, plus a new set of commands/handlers for managing resource mapping groups.

  • Extend CreateResourceMapping and UpdateResourceMapping APIs to include GroupId
  • Implement CRUD handlers and CLI commands for resource mapping groups
  • Update CLI docs to expose the new --group-id flag in resource-mappings commands

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
pkg/handlers/resourceMappings.go Added grpId parameter to create/update calls
pkg/handlers/resourceMappingGroups.go New handler methods for resource mapping group CRUD
cmd/policy-resourceMappings.go Wired --group-id flag through CLI commands & output tables
cmd/policy-resourceMappingGroups.go New CLI commands for group management (create/get/list/etc)
docs/man/policy/resource-mappings/*.md Added group-id flag docs for create/update
docs/man/policy/resource-mapping-groups/*.md Initial docs for resource mapping group commands
Comments suppressed due to low confidence (3)

cmd/policy-resourceMappingGroups.go:183

  • The list subcommand is wired to policy_listResourceMappings instead of policy_listResourceMappingGroups, causing a mismatch.
listDoc := man.Docs.GetCommand("policy/resource-mapping-groups/list", man.WithRun(policy_listResourceMappings))

pkg/handlers/resourceMappings.go:18

  • [nitpick] The parameter names are inconsistent with UpdateResourceMapping which uses attrValueId. Consider unifying on attributeValueId and using groupId instead of grpId for clarity.
func (h *Handler) CreateResourceMapping(attributeId string, terms []string, grpId string, metadata *common.MetadataMutable) (*policy.ResourceMapping, error) {

pkg/handlers/resourceMappingGroups.go:1

  • The new resource mapping groups handlers lack accompanying unit or integration tests. Consider adding tests to cover CRUD operations.
package handlers

@elizabethhealy elizabethhealy changed the title feat(cli): Resource mapping groups feat(core): Resource mapping groups Jun 2, 2025
@elizabethhealy elizabethhealy marked this pull request as ready for review June 2, 2025 16:57
@elizabethhealy elizabethhealy requested a review from a team as a code owner June 2, 2025 16:57
@elizabethhealy elizabethhealy merged commit 03fa307 into main Jun 2, 2025
12 checks passed
@elizabethhealy elizabethhealy deleted the dspx-1075-add-resource-mapping-groups branch June 2, 2025 19:07
jakedoublev added a commit that referenced this pull request Jun 3, 2025
🤖 I have created a release *beep* *boop*
---


##
[0.21.0](v0.20.0...v0.21.0)
(2025-05-29)


### Features

* Add initial Dependency Review configuration
([#551](#551))
([b622666](b622666))
* **core:** Add base key cmds
([#563](#563))
([edfd6c0](edfd6c0))
* **core:** DSPX-18 clean up Go context usage to follow best practices
([#558](#558))
([a2c9f8b](a2c9f8b))
* **core:** DSPX-608 - Deprecate public_client_id
([#555](#555))
([8d396bd](8d396bd))
* **core:** DSPX-608 - require clientID for login
([#553](#553))
([580172e](580172e))
* **core:** DSPX-896 add registered resources CRUD
([#559](#559))
([8e7475e](8e7475e))
* **core:** KAS allowlist options
([#539](#539))
([af7978f](af7978f))
* **core:** key management operations
([#533](#533))
([d4f6aaa](d4f6aaa))
* **main:** add actions CRUD and e2e tests
([#523](#523))
([2fb9ec7](2fb9ec7))
* **main:** refactor actions within existing CLI policy object CRUD
([#543](#543))
([9ab1a58](9ab1a58))
* **core:** Resource mapping groups
([#567](#567))
([03fa307](03fa307))
* **core:** Update key mgmt flags to consistent format
([#570](#570))
([#846f96c](846f96c))
* **core:** Rotate Key
([#572](#572))
([afd0043](afd0043))


### Bug Fixes

* **ci:** ci job should run on changes to GHA
([#530](#530))
([1d296ca](1d296ca))
* **main:** Pass the full url when building the sdk object
([#544](#544))
([8b836f0](8b836f0))

---
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>
Co-authored-by: Elizabeth Healy <[email protected]>
Co-authored-by: Chris Reed <[email protected]>
Co-authored-by: Jake Van Vorhis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants