Skip to content

Conversation

@huerni
Copy link
Collaborator

@huerni huerni commented Dec 2, 2025

Summary by CodeRabbit

  • New Features

    • Added license resource management: create, modify (also aliased as "update"), delete, and query resources with per-cluster settings, operator fields, and new CLI flags for name/server/clusters/allowed.
  • UI / Display

    • Improved license listing: remote metrics (last consumed, deficit), formatted last-updated timestamps, optional cluster breakdowns and table-style rendering.
  • Monitoring

    • License usage telemetry now includes reserved, last_consumed, and last_deficit.
  • Errors

    • Clearer resource-related error messages for invalid input, not found, and already exists.
  • Validation

    • Tightened license string parsing and added input validation for resource operations.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 2, 2025

📝 Walkthrough

Walkthrough

Adds license-resource support end-to-end: protobuf messages and RPCs for license resources, CLI parser/flags/handlers for a new "resource" entity, cacctmgr implementations for resource CRUD and printing, error mappings, and enhanced license display for remote licenses.

Changes

Cohort / File(s) Summary
Protobuf API (messages & RPCs)
protos/Crane.proto, protos/PublicDefs.proto
Added messages for add/modify/delete/query license resources, LicenseResource/LicenseResourceInfo, new ErrCode values (ERR_RESOURCE_NOT_FOUND, ERR_INVALID_ARGUMENT, ERR_RESOURCE_ALREADY_EXIST), extended LicenseInfo fields, and RPCs: AddOrModifyLicenseResource, DeleteLicenseResource, QueryLicenseResource.
CACCTMgr logic (resource management)
internal/cacctmgr/cacctmgr.go
New functions: PrintLicenseResource, AddLicenseResource, DeleteLicenseResource, ModifyResource, ShowLicenseResources — cluster parsing, operator mapping, RPC request/response handling, JSON/text output, and table rendering.
CLI parser & command dispatch
internal/cacctmgr/parser.go, internal/cacctmgr/cmd.go
Added resource entity and `modify
License output formatting
internal/ccontrol/output.go
ShowLicenses now branches on remote: non-remote keeps prior fields plus Reserved/Remote=no; remote shows LastConsumed, LastDeficit, formatted LastUpdated, and Remote=yes.
Error messages
internal/util/err.go
Added human-readable mappings for new license/resource-related error codes.
Utilities / parsing
internal/util/string.go
Tightened ParseLicensesString regex to ^([a-zA-Z]+(?:@[a-zA-Z]+)?):(\d+)$, restricting accepted license-name formats.
Monitoring plugin
plugin/monitor/pkg/db/influxdb.go
SaveLicenseUsage now includes reserved, last_deficit, and last_consumed fields in the InfluxDB point for license usage.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant CLI as Client (cacctmgr)
    participant Parser as CLI Parser
    participant CACCTMgr as cacctmgr
    participant Ctld as CraneCtld (gRPC)
    participant Store as Backend/DB

    CLI->>Parser: parse "add|modify|update|delete|show resource ..." (flags, KV params)
    Parser-->>CLI: command struct
    CLI->>CACCTMgr: call Add/Modify/Delete/ShowLicenseResources(...)
    note right of CACCTMgr `#e6f4ea`: build protobuf request\n(parse clusters, operators)
    CACCTMgr->>Ctld: gRPC call (AddOrModify / Delete / Query)
    Ctld->>Store: persist/lookup resource, validate clusters/duplicates
    Store-->>Ctld: reply (ok / rich_err / resource list)
    Ctld-->>CACCTMgr: RPC reply (ok, rich_err, license_resource_list)
    CACCTMgr-->>CLI: print JSON or formatted table (includes remote license fields when present)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

enhancement

Suggested reviewers

  • Nativu5
  • L-Xiafeng

Poem

"I hopped through code with nimble paws,
New resources sprouting like clover laws.
RPCs and flags in tidy rows,
Tables sorted where the green grass grows.
A rabbit cheers — deploy, rejoice, applause!" 🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The PR title 'feat: remote license' is vague and does not clearly convey the main changes, which involve adding comprehensive license resource management capabilities (CRUD operations, CLI commands, protobuf messages) rather than just 'remote license' functionality. Consider a more descriptive title that better captures the scope, such as 'feat: add license resource management with CRUD operations and CLI support' or 'feat: implement license resource management commands and protobuf APIs'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dev/remote_license

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a05dc08 and 4464714.

📒 Files selected for processing (3)
  • internal/util/err.go
  • plugin/monitor/pkg/db/influxdb.go
  • protos/PublicDefs.proto
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/util/err.go
🔇 Additional comments (5)
protos/PublicDefs.proto (3)

693-696: LGTM!

The new error codes follow the established naming conventions and sequential numbering pattern. They appropriately support the new license resource management functionality.


837-848: LGTM!

The LicenseResourceInfo message is well-structured with proper snake_case naming conventions throughout. The field types are appropriate for tracking license resource metadata across clusters.


850-870: LGTM!

The LicenseResource message effectively serves as a namespace for related enums. The enum definitions follow protobuf best practices with zero-value defaults (NotSet, None) and descriptive member names.

plugin/monitor/pkg/db/influxdb.go (2)

296-301: The extended license metrics look correct.

The additional fields (reserved, last_deficit, last_consumed) are properly added to the InfluxDB data point and correctly access the corresponding LicenseInfo proto fields. The field names in the InfluxDB storage use proper snake_case naming.

Note: If the proto field naming is corrected per the comment on PublicDefs.proto, the Go accessor names will remain the same (proto generator converts snake_case to PascalCase), so no changes would be needed here.


9-10: Both library versions are already current and secure. influxdb-client-go/v2 v2.14.0 is the latest stable release (Aug 2024), and logrus v1.9.3 is the latest stable release (June 2025) which includes the fix for CVE-2025-65637. No action required.

Likely an incorrect or invalid review comment.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 9

🧹 Nitpick comments (7)
protos/PublicDefs.proto (2)

821-823: Inconsistent field naming convention.

The fields LastConsumed, LastDeficit, and LastUpdate use PascalCase, while other fields in this message and throughout the proto files use snake_case (e.g., reserved, remote). This inconsistency also differs from the snake_case last_consumed field in LicenseResourceInfo at line 846.

Apply this diff to follow protobuf naming conventions:

-  uint32 LastConsumed = 7;
-  uint32 LastDeficit = 8;
-  int64 LastUpdate = 9;
+  uint32 last_consumed = 7;
+  uint32 last_deficit = 8;
+  int64 last_update = 9;

Note: This will require updating the Go code in output.go to use the generated accessor names (e.g., licenseInfo.LastConsumedlicenseInfo.GetLastConsumed()).


695-697: Inconsistent naming: missing trailing 'S' in ERR_RESOURCE_ALREADY_EXIST.

Other similar error codes use EXISTS (e.g., ERR_USER_ALREADY_EXISTS, ERR_ACCOUNT_ALREADY_EXISTS, ERR_QOS_ALREADY_EXISTS). Consider renaming for consistency.

-  ERR_RESOURCE_ALREADY_EXIST = 85;
+  ERR_RESOURCE_ALREADY_EXISTS = 85;
internal/cacctmgr/cmd.go (1)

435-444: Missing validation for unknown parameters.

Unlike other delete commands (e.g., executeDeleteUserCommand), unknown keys are silently ignored here. Consider adding a default case for consistency.

 	for key, value := range KVParams {
 		switch strings.ToLower(key) {
 		case "name":
 			FlagEntityName = value
 		case "server":
 			FlagServer = value
 		case "cluster":
 			FlagClusters = value
+		default:
+			log.Errorf("unknown flag: %s", key)
+			return util.ErrorCmdArg
 		}
 	}
internal/cacctmgr/cacctmgr.go (3)

140-140: Use idiomatic Go naming: clusterName instead of cluster_name.

-		for cluster_name, allowed := range info.ClusterResourceInfo {
+		for clusterName, allowed := range info.ClusterResourceInfo {

And update the reference on line 154 accordingly.


346-349: Typo: filed should be field.

-	for filed, value := range operators {
-		req.Operators = append(req.Operators, &protos.AddOrModifyLicenseResourceRequest_Operator{OperatorField: filed, Value: value})
+	for field, value := range operators {
+		req.Operators = append(req.Operators, &protos.AddOrModifyLicenseResourceRequest_Operator{OperatorField: field, Value: value})
	}

728-731: Typo: filed should be field.

Same typo as in AddLicenseResource.

-	for filed, value := range operators {
-		req.Operators = append(req.Operators, &protos.AddOrModifyLicenseResourceRequest_Operator{OperatorField: filed, Value: value})
+	for field, value := range operators {
+		req.Operators = append(req.Operators, &protos.AddOrModifyLicenseResourceRequest_Operator{OperatorField: field, Value: value})
	}
internal/cacctmgr/parser.go (1)

286-287: Add ModifyCommand handling to GetKVParamValue for consistency with GetKVMaps.

GetKVMaps handles ModifyCommand (lines 286-287) and is actively used when processing modify commands (e.g., line 875 in executeModifyResourceCommand). However, GetKVParamValue does not include ModifyCommand in its switch statement. Since ModifyCommand has KVParams and GetKVMaps processes it, GetKVParamValue should handle it for API consistency. If future code calls GetKVParamValue on a ModifyCommand, it will incorrectly return an empty string.

Add the following case to the switch statement in GetKVParamValue (after line 237):

	case ModifyCommand:
		params = cmd.KVParams
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5788342 and 2421e2a.

📒 Files selected for processing (7)
  • internal/cacctmgr/cacctmgr.go (5 hunks)
  • internal/cacctmgr/cmd.go (10 hunks)
  • internal/cacctmgr/parser.go (4 hunks)
  • internal/ccontrol/output.go (1 hunks)
  • internal/util/err.go (1 hunks)
  • protos/Crane.proto (2 hunks)
  • protos/PublicDefs.proto (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/cacctmgr/cmd.go (3)
internal/cacctmgr/parser.go (1)
  • CAcctMgrCommand (32-34)
internal/util/err.go (2)
  • ErrorSuccess (34-34)
  • ErrorCmdArg (36-36)
internal/cacctmgr/cacctmgr.go (4)
  • AddLicenseResource (319-377)
  • DeleteLicenseResource (499-542)
  • ModifyResource (705-758)
  • ShowLicenseResources (805-847)
internal/cacctmgr/parser.go (2)
internal/ccontrol/parser.go (2)
  • EntityType (78-84)
  • KeyValueParam (73-76)
internal/ccon/flags.go (1)
  • GlobalFlags (25-29)
🔇 Additional comments (8)
protos/PublicDefs.proto (1)

838-870: LGTM!

The LicenseResourceInfo and LicenseResource message structures are well-designed for representing license resources with cluster-level quotas. The nested enums provide clear type safety for field operations.

protos/Crane.proto (2)

986-1027: LGTM!

The new license resource management messages follow the existing patterns in this proto file. The AddOrModifyLicenseResourceRequest design with an is_add flag and flexible Operator list for field-level updates is clean and extensible.


1074-1077: LGTM!

The new RPC endpoints are appropriately placed under the /* RPCs called from cacctmgr */ section and follow the existing naming conventions.

internal/cacctmgr/cmd.go (1)

41-44: LGTM!

The new flags and the "update" alias for executeModifyCommand follow existing patterns in this file, providing flexibility for users familiar with either verb.

Also applies to: 105-105

internal/cacctmgr/cacctmgr.go (1)

805-847: LGTM!

The function follows the established pattern for query operations with proper error handling and output formatting.

internal/cacctmgr/parser.go (3)

68-75: LGTM!

The addition of the update alias and KVParams field properly extends ModifyCommand to support the new resource management flow.


93-99: LGTM!

The Resource field extends EntityType consistently with existing entity types.


164-165: LGTM!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (6)
internal/cacctmgr/cmd.go (3)

421-446: Consider using package-level FlagServerName for consistency.

Line 423 declares a local FlagServer variable, while other delete functions use package-level flags like FlagEntityName. For consistency with the rest of the codebase and with the add/modify/show resource functions that use FlagServerName, consider using the existing package-level variable instead.

Apply this diff:

 func executeDeleteResourceCommand(command *CAcctMgrCommand) int {
 	FlagEntityName = command.GetID()
-	FlagServer := ""
+	FlagServerName = ""
 	if FlagEntityName == "" {
 		log.Errorf("Error: required entity resource not set")
 		return util.ErrorCmdArg
 	}
 	KVParams := command.GetKVMaps()
 
 	err := checkEmptyKVParams(KVParams, []string{"server"})
 	if err != util.ErrorSuccess {
 		return err
 	}
 
 	for key, value := range KVParams {
 		switch strings.ToLower(key) {
 		case "name":
 			FlagEntityName = value
 		case "server":
-			FlagServer = value
+			FlagServerName = value
 		case "cluster":
 			FlagClusters = value
 		}
 	}
-	return DeleteLicenseResource(FlagEntityName, FlagServer, FlagClusters)
+	return DeleteLicenseResource(FlagEntityName, FlagServerName, FlagClusters)
 }

871-967: Clarify the dual-parsing logic for KvParams and WhereParams.

The function accepts resource identifiers through both KvParams and WhereParams, parsing the same fields (name, server, cluster) twice. While this flexibility may be intentional to support different command syntaxes, the error message on line 880 only mentions the 'where' clause, which is misleading.

Consider either:

  1. Updating the error message to reflect that both approaches are acceptable
  2. Documenting why both parsing paths exist
  3. Simplifying to use only one approach (WhereParams) for consistency with other modify commands

Example error message improvement:

 	if len(KvParams) == 0 && len(WhereParams) == 0 {
-		log.Errorf("Error: modify resource command requires 'where' clause to specify which resource to modify")
+		log.Errorf("Error: modify resource command requires resource identifiers (name, server) to specify which resource to modify")
 		return util.ErrorCmdArg
 	}

1061-1088: Consider consistency improvements for flag handling.

Two minor consistency suggestions:

  1. Line 1065: Similar to executeDeleteResourceCommand, this declares a local FlagServer variable instead of using the package-level FlagServerName.

  2. Lines 1063 & 1082-1085: Using command.GetID() to capture an optional "withclusters" modifier and storing it in FlagWithClusters (a string) before converting to hasWithClusters (a boolean) is unconventional. Consider renaming FlagWithClusters to withClustersStr or similar to clarify it's not a boolean flag.

Apply this diff:

 func executeShowResourceCommand(command *CAcctMgrCommand) int {
 
-	FlagWithClusters := command.GetID()
+	withClustersStr := command.GetID()
 
-	FlagServer := ""
+	FlagServerName = ""
 
 	WhereParams := command.GetWhereParams()
 	for key, value := range WhereParams {
 		switch strings.ToLower(key) {
 		case "name":
 			FlagEntityName = value
 		case "server":
-			FlagServer = value
+			FlagServerName = value
 		case "cluster":
 			FlagClusters = value
 		default:
 			log.Errorf("Error: unknown where parameter '%s' for show resource", key)
 			return util.ErrorCmdArg
 		}
 	}
 
 	hasWithClusters := false
-	if strings.ToLower(FlagWithClusters) == "withclusters" {
+	if strings.ToLower(withClustersStr) == "withclusters" {
 		hasWithClusters = true
 	}
 
-	return ShowLicenseResources(FlagEntityName, FlagServer, FlagClusters, hasWithClusters)
+	return ShowLicenseResources(FlagEntityName, FlagServerName, FlagClusters, hasWithClusters)
 }
internal/cacctmgr/cacctmgr.go (3)

93-168: Use camelCase for Go variable names.

Line 140 uses cluster_name (snake_case) instead of the Go convention clusterName (camelCase).

Apply this diff:

 		} else {
-			for cluster_name, allowed := range info.ClusterResourceInfo {
+			for clusterName, allowed := range info.ClusterResourceInfo {
 				var allowedString string
 				allowedString = fmt.Sprintf("%d", allowed)
 				if info.Flags == uint32(protos.LicenseResource_None) {
 					allowedString += "%"
 				}
 				tableData = append(tableData, []string{
 					info.ResourceName,
 					info.Server,
 					typeString,
 					strconv.Itoa(int(info.Count)),
 					strconv.Itoa(int(info.LastConsumed)),
 					allocatedString,
 					info.ServerType,
-					cluster_name,
+					clusterName,
 					allowedString,
 					flagString,
 				})
 			}
 		}

319-377: Fix typo: filed should be field.

Line 347 has a typo in the loop variable name.

Apply this diff:

 	req.Operators = make([]*protos.AddOrModifyLicenseResourceRequest_Operator, 0)
-	for filed, value := range operators {
-		req.Operators = append(req.Operators, &protos.AddOrModifyLicenseResourceRequest_Operator{OperatorField: filed, Value: value})
+	for field, value := range operators {
+		req.Operators = append(req.Operators, &protos.AddOrModifyLicenseResourceRequest_Operator{OperatorField: field, Value: value})
 	}

705-758: Fix typo: filed should be field.

Line 729 has the same typo as in AddLicenseResource.

Apply this diff:

 	req.Operators = make([]*protos.AddOrModifyLicenseResourceRequest_Operator, 0)
-	for filed, value := range operators {
-		req.Operators = append(req.Operators, &protos.AddOrModifyLicenseResourceRequest_Operator{OperatorField: filed, Value: value})
+	for field, value := range operators {
+		req.Operators = append(req.Operators, &protos.AddOrModifyLicenseResourceRequest_Operator{OperatorField: field, Value: value})
 	}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2421e2a and 57c1b91.

📒 Files selected for processing (4)
  • internal/cacctmgr/cacctmgr.go (5 hunks)
  • internal/cacctmgr/cmd.go (10 hunks)
  • internal/ccontrol/output.go (1 hunks)
  • internal/util/err.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • internal/util/err.go
  • internal/ccontrol/output.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/cacctmgr/cacctmgr.go (4)
internal/util/table.go (2)
  • SetBorderTable (41-49)
  • TrimTable (121-123)
internal/cacctmgr/cmd.go (4)
  • FlagFull (66-66)
  • FlagFormat (94-94)
  • FlagForce (65-65)
  • FlagJson (67-67)
internal/util/string.go (2)
  • CheckEntityName (609-629)
  • ParseStringParamList (1132-1141)
internal/util/formatter.go (1)
  • FmtJson (58-58)
🔇 Additional comments (3)
internal/cacctmgr/cmd.go (1)

280-338: LGTM - Well-structured resource addition handler.

The function correctly validates required parameters, enforces the cluster/allowed pairing constraint, and uses appropriate 32-bit validation for numeric fields.

internal/cacctmgr/cacctmgr.go (2)

499-542: LGTM - Resource deletion handler is well-structured.

The function follows the established pattern of other delete handlers, properly validates cluster parameters, and provides clear error messages.


805-847: LGTM - Resource query handler is well-implemented.

The function properly validates input, handles errors comprehensively, supports both JSON and text output, and integrates cleanly with PrintLicenseResource.

@huerni huerni force-pushed the dev/remote_license branch from 57c1b91 to 829fdbe Compare December 23, 2025 09:13
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/cacctmgr/parser.go (1)

1-1: Fix gofmt formatting issue.

The pipeline indicates this file is not properly formatted. Run gofmt -w internal/cacctmgr/parser.go to fix the formatting.

🧹 Nitpick comments (8)
internal/util/err.go (1)

167-170: Minor inconsistency in article usage.

The error messages use inconsistent phrasing: "in Crane" vs "in the Crane". Consider aligning them for consistency.

 	// LicenseResource errors
 	protos.ErrCode_ERR_INVALID_ARGUMENT:       "Invalid argument provided",
 	protos.ErrCode_ERR_LICENSE_NOT_FOUND:      "The resource was not found in Crane",
-	protos.ErrCode_ERR_RESOURCE_ALREADY_EXIST: "The resource already exists in the Crane",
+	protos.ErrCode_ERR_RESOURCE_ALREADY_EXIST: "The resource already exists in Crane",
internal/cacctmgr/cmd.go (2)

435-444: Missing error handling for unknown parameters.

Unlike executeDeleteUserCommand (which logs errors for unknown flags), this function silently ignores unknown parameters. Consider adding a default case to log unknown parameters for consistency.

🔎 Suggested fix
 	for key, value := range KVParams {
 		switch strings.ToLower(key) {
 		case "name":
 			FlagEntityName = value
 		case "server":
 			FlagServer = value
 		case "cluster":
 			FlagClusters = value
+		default:
+			log.Errorf("unknown flag: %s", key)
+			return util.ErrorCmdArg
 		}
 	}

879-924: Consider consolidating duplicate parameter processing logic.

The function processes KvParams and WhereParams with nearly identical switch statements (lines 891-903 and 912-924). This could be simplified by merging the maps first or extracting a helper function.

That said, the current implementation is functionally correct.

protos/PublicDefs.proto (1)

818-822: Inconsistent field naming convention.

The new fields in LicenseInfo mix conventions: reserved and remote use lower_snake_case, while LastConsumed, LastDeficit, and LastUpdate use PascalCase. Proto3 convention typically uses lower_snake_case for field names.

🔎 Suggested fix
   uint32 reserved = 5;
   bool remote = 6;
-  uint32 LastConsumed = 7;
-  uint32 LastDeficit = 8;
-  int64 LastUpdate = 9;
+  uint32 last_consumed = 7;
+  uint32 last_deficit = 8;
+  int64 last_update = 9;

Note: This change would affect generated code and may require updates to consumers of these fields.

internal/cacctmgr/cacctmgr.go (3)

139-156: Minor: Use Go naming conventions for loop variable.

The variable cluster_name uses snake_case which is unconventional in Go. Consider using camelCase (clusterName) for consistency with Go style guidelines.

-		for cluster_name, allowed := range info.ClusterResourceInfo {
+		for clusterName, allowed := range info.ClusterResourceInfo {
 			var allowedString string
 			allowedString = fmt.Sprintf("%d", allowed)
 			if info.Flags == uint32(protos.LicenseResource_None) {
 				allowedString += "%"
 			}
 			tableData = append(tableData, []string{
 				info.ResourceName,
 				info.Server,
 				typeString,
 				strconv.Itoa(int(info.Count)),
 				strconv.Itoa(int(info.LastConsumed)),
 				allocatedString,
 				info.ServerType,
-				cluster_name,
+				clusterName,
 				allowedString,
 				flagString,
 			})

345-348: Typo in variable name: filed should be field.

The loop variable is named filed but should be field for clarity.

 	req.Operators = make([]*protos.AddOrModifyLicenseResourceRequest_Operator, 0)
-	for filed, value := range operators {
-		req.Operators = append(req.Operators, &protos.AddOrModifyLicenseResourceRequest_Operator{OperatorField: filed, Value: value})
+	for field, value := range operators {
+		req.Operators = append(req.Operators, &protos.AddOrModifyLicenseResourceRequest_Operator{OperatorField: field, Value: value})
 	}

727-730: Same typo: filed should be field.

Same typo as in AddLicenseResource.

 	req.Operators = make([]*protos.AddOrModifyLicenseResourceRequest_Operator, 0)
-	for filed, value := range operators {
-		req.Operators = append(req.Operators, &protos.AddOrModifyLicenseResourceRequest_Operator{OperatorField: filed, Value: value})
+	for field, value := range operators {
+		req.Operators = append(req.Operators, &protos.AddOrModifyLicenseResourceRequest_Operator{OperatorField: field, Value: value})
 	}
internal/cacctmgr/parser.go (1)

225-243: Inconsistency: GetKVParamValue doesn't handle ModifyCommand.

GetKVMaps was updated to handle ModifyCommand (line 289-290), but GetKVParamValue was not. For API consistency, consider adding the ModifyCommand case here as well.

🔎 Suggested fix
 	switch cmd := c.Command.(type) {
 	case AddCommand:
 		params = cmd.KVParams
 	case DeleteCommand:
 		params = cmd.KVParams
 	case BlockCommand:
 		params = cmd.KVParams
 	case UnblockCommand:
 		params = cmd.KVParams
 	case ShowCommand:
 		params = cmd.KVParams
 	case ResetCommand:
 		params = cmd.KVParams
+	case ModifyCommand:
+		params = cmd.KVParams
 	default:
 		return ""
 	}
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 57c1b91 and 829fdbe.

📒 Files selected for processing (7)
  • internal/cacctmgr/cacctmgr.go
  • internal/cacctmgr/cmd.go
  • internal/cacctmgr/parser.go
  • internal/ccontrol/output.go
  • internal/util/err.go
  • protos/Crane.proto
  • protos/PublicDefs.proto
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/ccontrol/output.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/cacctmgr/parser.go (2)
internal/ccontrol/parser.go (1)
  • KeyValueParam (73-76)
internal/ccon/flags.go (1)
  • GlobalFlags (25-29)
🪛 GitHub Actions: Go Code Quality Check
internal/cacctmgr/parser.go

[error] 1-1: gofmt formatting check failed. The file is not properly formatted. The following file is not properly formatted: internal/cacctmgr/parser.go. Please run 'gofmt -w .' to fix the formatting.

🔇 Additional comments (10)
internal/cacctmgr/cmd.go (3)

41-44: LGTM!

The new flags for resource management are properly declared at package level, consistent with the existing flag declarations.


280-338: LGTM!

The executeAddResourceCommand function properly validates input, enforces the constraint that cluster and allowed must be specified together, and delegates to AddLicenseResource.


1095-1122: LGTM!

The executeShowResourceCommand function correctly parses the withclusters modifier from the command ID position and applies filters from the where clause. The implementation is functional.

protos/PublicDefs.proto (1)

837-869: LGTM!

The new LicenseResourceInfo message and LicenseResource definition with nested enums are well-structured and follow protobuf conventions. The field numbering is sequential and the enum values cover the necessary operations.

internal/cacctmgr/cacctmgr.go (2)

498-541: LGTM!

The DeleteLicenseResource function follows the established pattern for delete operations with proper error handling and output formatting.


804-846: LGTM!

The ShowLicenseResources function correctly queries license resources and renders them using PrintLicenseResource, with proper JSON output support.

protos/Crane.proto (2)

1029-1070: LGTM!

The new protobuf messages for license resource management are well-structured:

  • AddOrModifyLicenseResourceRequest with nested Operator message for field updates
  • DeleteLicenseResourceRequest and QueryLicenseResourceRequest follow existing patterns
  • Reply messages consistently use RichError for error reporting

The API design consolidates add and modify into a single RPC via the is_add flag, which reduces service surface area.


1117-1119: LGTM!

The new RPCs are correctly added to the CraneCtld service under the cacctmgr section, following the established pattern for account/user/QoS operations.

internal/cacctmgr/parser.go (2)

68-75: LGTM!

The ModifyCommand extension correctly supports:

  • Alternative action keyword update for user convenience
  • KVParams for simpler command syntax alongside the existing where/set clauses

This provides flexibility for different command styles.


99-99: LGTM!

The Resource entity type is correctly added to EntityType, with corresponding String() method update at lines 167-168.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 829fdbe and aa6e6c7.

📒 Files selected for processing (1)
  • internal/cacctmgr/parser.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/cacctmgr/parser.go (1)
internal/ccontrol/parser.go (1)
  • KeyValueParam (73-76)
🔇 Additional comments (3)
internal/cacctmgr/parser.go (3)

68-75: LGTM on the ModifyCommand struct expansion.

The struct now properly supports both modify and update actions with the alternation syntax, and the optional Where/Set clauses alongside KVParams provide flexibility for different modification patterns.


93-100: LGTM on the Resource entity addition.

The new Resource field follows the established pattern for entity type parsing.


167-168: LGTM on the String() method update.

The Resource case is correctly added and returns the expected string representation.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (3)
internal/util/string.go (1)

551-553: Simplify the redundant check.

Since count is parsed as uint, it cannot be negative. The condition count <= 0 is equivalent to count == 0.

🔎 Proposed simplification
-		if count <= 0 {
+		if count == 0 {
 			return nil, hasPipe, fmt.Errorf("license count must > 0")
 		}
protos/PublicDefs.proto (2)

839-839: Clarify the map key for cluster_resource_info.

The map<string, uint32> cluster_resource_info field lacks documentation on what the string key represents (e.g., cluster name, cluster ID, partition). Adding a comment would improve maintainability and help API consumers understand the expected key format.

Example:

  // Maps cluster name to resource count
  map<string, uint32> cluster_resource_info = 2;

850-870: Add documentation for enums to clarify their purpose.

The LicenseResource message contains enums without explanatory comments. Specifically:

  • The Field enum's purpose is unclear (is it for field masks, selection, or validation?)
  • The Flag enum's Absolute value is not self-explanatory (does it mean absolute count vs. percentage, or absolute allocation vs. shared?)

Adding comments would improve API usability and maintainability.

Example:

message LicenseResource {
  enum Type{
    NotSet = 0;
    License = 1;
  }

  // Fields that can be modified in resource operations
  enum Field {
    Count = 0;
    Allowed = 1;
    LastConsumed = 2;
    Description = 3;
    Flags = 4;
    ServerType = 5;
    ResourceType = 6;
  }

  // Flags controlling resource allocation behavior
  enum Flag {
    None = 0;
    Absolute = 1;  // Resource count is absolute (vs. relative/shared)
  }
}
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aa6e6c7 and a05dc08.

📒 Files selected for processing (3)
  • internal/util/err.go
  • internal/util/string.go
  • protos/PublicDefs.proto

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.

2 participants