Skip to content

fix: add short ID resolution to remaining SDK methods (#475, #473, #471, #470, #453)#488

Merged
poyrazK merged 8 commits intomainfrom
fix/cli-short-id-resolution-v2
May 7, 2026
Merged

fix: add short ID resolution to remaining SDK methods (#475, #473, #471, #470, #453)#488
poyrazK merged 8 commits intomainfrom
fix/cli-short-id-resolution-v2

Conversation

@poyrazK
Copy link
Copy Markdown
Owner

@poyrazK poyrazK commented May 7, 2026

Summary

  • Add short ID resolution to snapshot, autoscaling, compute, kubernetes, and DNS SDK methods
  • CreateSnapshot now accepts volume ID or name
  • CreateScalingPolicy now accepts group ID or name
  • compute module: added resolveID to all instance methods
  • kubernetes module: added resolveID to all cluster methods
  • CreateDNSZone now accepts VPC ID or name

Issues Fixed

Test plan

  • go test ./pkg/sdk/... - SDK tests pass
  • go test ./cmd/cloud/... - CLI tests pass
  • go build ./...

Summary by CodeRabbit

  • New Features

    • Many commands now accept resource name or ID (instances, autoscaling groups, DNS zones/VPC, Kubernetes clusters, snapshots, volumes), making CLI usage more flexible.
    • Snapshot creation and volume listing now support context-aware operations (better cancellation/timeouts).
  • Tests

    • Test suites updated to exercise name-to-ID resolution flows for autoscaling, instances, CSI, and snapshot scenarios.

…, #470, #453)

- snapshot CreateSnapshot: now accepts volume ID or name
- autoscaling CreateScalingPolicy: now accepts group ID or name
- compute: added resolveID to GetInstance, StopInstance, TerminateInstance,
  GetInstanceLogs, ResizeInstance, GetInstanceStats, GetConsoleURL, UpdateInstanceMetadata
- kubernetes: added resolveID to all cluster methods (GetCluster, DeleteCluster,
  GetKubeconfig, RepairCluster, ScaleCluster, GetClusterHealth, UpgradeCluster,
  RotateSecrets, CreateBackup, RestoreBackup, AddNodeGroup, UpdateNodeGroupWithContext,
  DeleteNodeGroup)
- dns CreateDNSZone: now accepts VPC ID or name

Updated tests to handle the list-then-get resolution pattern in mocks
Copilot AI review requested due to automatic review settings May 7, 2026 18:29
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 7, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR updates SDK methods, CLI commands, and tests so many resource operations accept either an ID or a name (string) and resolve to concrete IDs via List...() + resolveID (context-aware variants added); tests and mocks updated to return list endpoints for resolution.

Changes

ID/Name Resolution Unification

Layer / File(s) Summary
Context helper
pkg/sdk/client.go
Added resolveIDWithContext(ctx, ...) to perform ID/name resolution using a context-aware list function.
Compute Instance Methods
pkg/sdk/compute.go
Instance methods add idOrName resolution helpers and resolve via ListInstances()/resolveIDWithContext before calling /instances/{id}/...; many context-aware variants and signature updates.
Kubernetes Cluster Methods
pkg/sdk/kubernetes.go
Cluster and node-group methods accept idOrName and resolve via ListClusters()/resolveID (context variants added) before calling UUID-scoped endpoints.
Autoscaling Policy Creation
pkg/sdk/autoscaling.go
CreateScalingPolicy now accepts groupIDOrName and resolves group ID via ListScalingGroups() + resolveID before POSTing policies.
DNS Zone Creation (SDK)
pkg/sdk/dns.go
CreateDNSZone now accepts vpcIDOrName *string and resolves it to a VPC ID via ListVPCs() + resolveID when provided.
DNS CLI change
cmd/cloud/dns.go
CLI create-zone now forwards --vpc-id as an optional *string to the SDK instead of resolving locally.
Snapshot Creation (SDK)
pkg/sdk/snapshot.go
CreateSnapshot now takes ctx and volumeIDOrName string, resolves via ListVolumesWithContext() + resolveIDWithContext, and posts snapshot with resolved volume_id; removed uuid import usage.
Snapshot CLI change
cmd/cloud/snapshot.go
snapshot create accepts `[volume-id
Volume Listing (context-aware)
pkg/sdk/volume.go
Added ListVolumesWithContext(ctx) used by snapshot resolution.
Test Mock Servers
internal/ccm/instances_test.go, internal/csi/driver_test.go, cmd/cloud/autoscaling_cli_test.go
Test HTTP handlers extended to serve GET /instances, GET /instances/{id}, and GET /autoscaling/groups list endpoints to support client-side ID/name resolution in tests.
Compute Test Updates
pkg/sdk/compute_test.go
Tests updated with mock instance fixtures and conditional GET /instances branches before asserting resource-specific endpoints (stop, terminate, logs, stats, resize, error cases).
Snapshot Test Updates
pkg/sdk/snapshot_test.go
Tests now use string volume IDs; server asserts volume_id as string and CreateSnapshot calls updated to include context and string IDs.

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • poyrazK/thecloud#84: Related changes around ID-or-name resolution and instance listing APIs used by CCM and SDK helpers.
  • poyrazK/thecloud#79: Related CSI/driver test updates that modify mock server instance endpoints and instance-resolution behavior.

Poem

🐰 I hopped through lists and names today,

Resolved IDs the rabbit way.
From snapshots to DNS zones bright,
Names or UUIDs — both now take flight.
Hooray! 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: add short ID resolution to remaining SDK methods' accurately and concisely describes the main change across multiple SDK methods for accepting both IDs and names.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/cli-short-id-resolution-v2

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
Copy Markdown

@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

Caution

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

⚠️ Outside diff range comments (4)
pkg/sdk/kubernetes.go (2)

102-306: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Extract a resolveClusterID helper to eliminate 13× boilerplate duplication.

The identical resolveID closure block is duplicated across 13 methods in this file (GetClusterWithContext, DeleteCluster, GetKubeconfig, RepairCluster, ScaleCluster, GetClusterHealth, UpgradeCluster, RotateSecrets, CreateBackup, RestoreBackup, AddNodeGroup, UpdateNodeGroupWithContext, DeleteNodeGroup). This makes it the largest single source of duplication in the PR. Any future change — e.g., adding a UUID short-circuit, switching list pagination, supporting a third lookup key — would require editing 13 sites with high drift risk.

♻️ Suggested helper
// resolveClusterID resolves an ID or name to a concrete cluster UUID string.
func (c *Client) resolveClusterID(idOrName string) (string, error) {
	return c.resolveClusterIDWithContext(context.Background(), idOrName)
}

func (c *Client) resolveClusterIDWithContext(ctx context.Context, idOrName string) (string, error) {
	return c.resolveID("cluster", func() ([]interface{}, error) {
		clusters, err := c.ListClustersWithContext(ctx)
		return interfaceSlicePtr(clusters), err
	},
		func(v interface{}) string { return v.(*Cluster).ID.String() },
		func(v interface{}) string { return v.(*Cluster).Name },
		idOrName)
}

Each call site collapses to:

 func (c *Client) ScaleCluster(idOrName string, workers int) error {
-	id, err := c.resolveID("cluster", func() ([]interface{}, error) {
-		clusters, err := c.ListClusters()
-		return interfaceSlicePtr(clusters), err
-	}, func(v interface{}) string { return v.(*Cluster).ID.String() }, func(v interface{}) string { return v.(*Cluster).Name }, idOrName)
+	id, err := c.resolveClusterID(idOrName)
 	if err != nil {
 		return err
 	}
 	var resp Response[any]
 	input := &ScaleClusterInput{Workers: workers}
 	return c.post(clustersPath+"/"+id+"/scale", input, &resp)
 }

This pairs naturally with the context-propagation fix above.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/sdk/kubernetes.go` around lines 102 - 306, The duplicated resolveID
closure used in GetClusterWithContext, DeleteCluster, GetKubeconfig,
RepairCluster, ScaleCluster, GetClusterHealth, UpgradeCluster, RotateSecrets,
CreateBackup, RestoreBackup, AddNodeGroup, UpdateNodeGroupWithContext and
DeleteNodeGroup should be extracted into a small helper (e.g.,
Client.resolveClusterID and resolveClusterIDWithContext) that calls
c.resolveID("cluster", ... ) and uses ListClustersWithContext when given a
context; replace each site to call the new helper (or the context variant in
UpdateNodeGroupWithContext/GetClusterWithContext) so all 13 call sites remove
the repeated closure and propagate context where appropriate.

108-121: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Context not propagated in *WithContext methods.

GetClusterWithContext and UpdateNodeGroupWithContext accept ctx but invoke c.ListClusters() inside the resolution closure without propagating it. This breaks context semantics—cancellation and deadlines don't apply to the blocking list operation. Create ListClustersWithContext and use it in both methods to propagate the context properly.

Proposed fix

Add the context-aware variant:

 // ListClusters returns all clusters for the current user.
 func (c *Client) ListClusters() ([]*Cluster, error) {
-	var resp Response[[]*Cluster]
-	if err := c.get(clustersPath, &resp); err != nil {
-		return nil, err
-	}
-	return resp.Data, nil
+	return c.ListClustersWithContext(context.Background())
+}
+
+// ListClustersWithContext returns all clusters with context support.
+func (c *Client) ListClustersWithContext(ctx context.Context) ([]*Cluster, error) {
+	var resp Response[[]*Cluster]
+	if err := c.getWithContext(ctx, clustersPath, &resp); err != nil {
+		return nil, err
+	}
+	return resp.Data, nil
 }

Update both methods to use it:

-		clusters, err := c.ListClusters()
+		clusters, err := c.ListClustersWithContext(ctx)

Applies to lines 110 and 282.

Violates: "Propagate context.Context to all blocking calls".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/sdk/kubernetes.go` around lines 108 - 121, GetClusterWithContext (and
UpdateNodeGroupWithContext) accept ctx but call c.ListClusters() inside the
resolveID closure, so the context isn't propagated; add a new method
ListClustersWithContext(ctx context.Context) that mirrors ListClusters but calls
the underlying request with context, then update the resolveID closures in
GetClusterWithContext and UpdateNodeGroupWithContext to call
c.ListClustersWithContext(ctx) instead of c.ListClusters() so cancellation and
deadlines apply to the listing operation.
pkg/sdk/compute.go (1)

49-224: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Extract a resolveInstanceID helper to eliminate 9× boilerplate duplication.

The same 5-line resolveID closure block is repeated verbatim in 9 methods (GetInstance, GetInstanceWithContext, GetConsoleURL, UpdateInstanceMetadata, StopInstance, TerminateInstanceWithContext, GetInstanceLogs, ResizeInstance, GetInstanceStats). Any future schema change (e.g., Instance.ID becoming uuid.UUID, adding a short-ID matcher, or caching the list) currently requires editing 9 sites with high risk of drift.

♻️ Suggested helper
// resolveInstanceID resolves an ID or name to a concrete instance ID.
func (c *Client) resolveInstanceID(idOrName string) (string, error) {
	return c.resolveID("instance", func() ([]interface{}, error) {
		instances, err := c.ListInstances()
		return interfaceSlice(instances), err
	},
		func(v interface{}) string { return v.(Instance).ID },
		func(v interface{}) string { return v.(Instance).Name },
		idOrName)
}

// And a context-aware variant for the WithContext methods:
func (c *Client) resolveInstanceIDWithContext(ctx context.Context, idOrName string) (string, error) {
	return c.resolveID("instance", func() ([]interface{}, error) {
		instances, err := c.ListInstancesWithContext(ctx)
		return interfaceSlice(instances), err
	},
		func(v interface{}) string { return v.(Instance).ID },
		func(v interface{}) string { return v.(Instance).Name },
		idOrName)
}

Each call site collapses to 2 lines:

 func (c *Client) StopInstance(idOrName string) error {
-	id, err := c.resolveID("instance", func() ([]interface{}, error) {
-		instances, err := c.ListInstances()
-		return interfaceSlice(instances), err
-	}, func(v interface{}) string { return v.(Instance).ID }, func(v interface{}) string { return v.(Instance).Name }, idOrName)
+	id, err := c.resolveInstanceID(idOrName)
 	if err != nil {
 		return err
 	}
 	return c.post(fmt.Sprintf("/instances/%s/stop", id), nil, nil)
 }

Since the project is on Go 1.25 (per the go.mod / library context), making resolveID itself generic would also remove the interface{} shuffle and the unchecked type assertions.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/sdk/compute.go` around lines 49 - 224, Extract a helper that centralizes
the repeated resolveID closure: add methods resolveInstanceID(idOrName string)
(string, error) and resolveInstanceIDWithContext(ctx context.Context, idOrName
string) (string, error) which call c.resolveID("instance", func()
([]interface{}, error) { instances, err := c.ListInstances(); return
interfaceSlice(instances), err }, func(v interface{}) string { return
v.(Instance).ID }, func(v interface{}) string { return v.(Instance).Name },
idOrName) and the context variant should call ListInstancesWithContext(ctx);
then replace the 9 duplicated resolveID closures in GetInstance,
GetInstanceWithContext, GetConsoleURL, UpdateInstanceMetadata, StopInstance,
TerminateInstanceWithContext, GetInstanceLogs, ResizeInstance, and
GetInstanceStats to call the new helpers (use the context variant in methods
with ctx) so type assertions and listing logic live in one place.
cmd/cloud/snapshot.go (1)

62-63: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update the command usage text to reflect ID-or-name input.

CreateSnapshot now accepts either volume ID or name, but the CLI help still advertises only [volume-id]. This can mislead users.

Suggested patch
-var snapshotCreateCmd = &cobra.Command{
-	Use:   "create [volume-id]",
+var snapshotCreateCmd = &cobra.Command{
+	Use:   "create [volume-id-or-name]",
 	Short: "Create a snapshot from a volume",
 	Args:  cobra.ExactArgs(1),
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/cloud/snapshot.go` around lines 62 - 63, Update the Cobra command usage
string to indicate the command accepts either a volume ID or name: locate the
command definition for CreateSnapshot (the block where Use: "create [volume-id]"
and Short: "Create a snapshot from a volume" are set) and change the Use value
to something like "create [volume-id|volume-name]" (or "create
<volume-id|volume-name>"); also update any related Short or Long help text if
present to mention “ID or name” so CLI help accurately reflects accepted input.
🧹 Nitpick comments (1)
pkg/sdk/snapshot_test.go (1)

26-66: ⚡ Quick win

Add a name-based case for CreateSnapshot (table-driven).

This test validates ID input, but the feature change is “ID or name”. A table-driven test with both volumeIDStr and "test-volume" would lock in the new behavior and prevent regressions.

As per coding guidelines, "Use table-driven tests in test files".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/sdk/snapshot_test.go` around lines 26 - 66, Update
TestClientCreateSnapshot to be table-driven and cover both identifier and name
lookup: create a table of cases with input strings {volumeIDStr, "test-volume"}
and for each subtest call client.CreateSnapshot(case.input,
snapshotDescription). Reuse the existing httptest server but ensure the GET
/volumes handler returns the volume with Name "test-volume" and ID volumeID so
name-based lookups succeed; in each subtest assert no error and that the
returned snapshot.VolumeID equals the canonical volumeID
(expectedSnapshot.VolumeID) and other fields match. Keep the existing request
body assertions but compare req["volume_id"] to case.input so the test verifies
both ID-passthrough and name-passthrough behavior in CreateSnapshot.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/sdk/compute.go`:
- Around line 65-78: GetInstanceWithContext currently calls c.ListInstances()
inside resolveID which drops the provided ctx; change the resolver to call
c.ListInstancesWithContext(ctx) so the parent's context (and its
deadline/cancellation) is propagated; do the same fix in
TerminateInstanceWithContext (replace uses of c.ListInstances() in resolveID
with c.ListInstancesWithContext(ctx)). Ensure you still pass the same resolver
functions (ID/Name extractors) and keep using c.getWithContext for the final
fetch.

In `@pkg/sdk/snapshot_test.go`:
- Around line 40-42: The handler in snapshot_test.go currently ignores the error
returned by json.NewEncoder(w).Encode(...) which can hide encoding failures;
update the handler to capture the returned error (e.g., err :=
json.NewEncoder(w).Encode(...)) and handle it instead of assigning to the blank
identifier — in a test handler either fail the test (t.Fatalf/require.NoError if
t is in scope) or write an appropriate HTTP 500 via http.Error(w, ...) and log
the error so encoding failures are visible; locate the Encode call in the test
HTTP handler and replace the blank-identifier usage with explicit error
handling.

In `@pkg/sdk/snapshot.go`:
- Around line 10-25: The CreateSnapshot method currently lacks a context.Context
parameter and must accept and propagate it: change CreateSnapshot to begin with
ctx context.Context and update its use of resolveID, ListVolumes and post to
pass ctx through (e.g., call c.resolveID(ctx, ...), c.ListVolumes(ctx) inside
the resolver, and c.post(ctx, "/snapshots", req, &snapshot)). Also update the
signatures of resolveID, ListVolumes and post (and any other downstream helpers
invoked) to accept context.Context as their first parameter so the
list-then-post flow is cancellable and respects timeouts.

---

Outside diff comments:
In `@cmd/cloud/snapshot.go`:
- Around line 62-63: Update the Cobra command usage string to indicate the
command accepts either a volume ID or name: locate the command definition for
CreateSnapshot (the block where Use: "create [volume-id]" and Short: "Create a
snapshot from a volume" are set) and change the Use value to something like
"create [volume-id|volume-name]" (or "create <volume-id|volume-name>"); also
update any related Short or Long help text if present to mention “ID or name” so
CLI help accurately reflects accepted input.

In `@pkg/sdk/compute.go`:
- Around line 49-224: Extract a helper that centralizes the repeated resolveID
closure: add methods resolveInstanceID(idOrName string) (string, error) and
resolveInstanceIDWithContext(ctx context.Context, idOrName string) (string,
error) which call c.resolveID("instance", func() ([]interface{}, error) {
instances, err := c.ListInstances(); return interfaceSlice(instances), err },
func(v interface{}) string { return v.(Instance).ID }, func(v interface{})
string { return v.(Instance).Name }, idOrName) and the context variant should
call ListInstancesWithContext(ctx); then replace the 9 duplicated resolveID
closures in GetInstance, GetInstanceWithContext, GetConsoleURL,
UpdateInstanceMetadata, StopInstance, TerminateInstanceWithContext,
GetInstanceLogs, ResizeInstance, and GetInstanceStats to call the new helpers
(use the context variant in methods with ctx) so type assertions and listing
logic live in one place.

In `@pkg/sdk/kubernetes.go`:
- Around line 102-306: The duplicated resolveID closure used in
GetClusterWithContext, DeleteCluster, GetKubeconfig, RepairCluster,
ScaleCluster, GetClusterHealth, UpgradeCluster, RotateSecrets, CreateBackup,
RestoreBackup, AddNodeGroup, UpdateNodeGroupWithContext and DeleteNodeGroup
should be extracted into a small helper (e.g., Client.resolveClusterID and
resolveClusterIDWithContext) that calls c.resolveID("cluster", ... ) and uses
ListClustersWithContext when given a context; replace each site to call the new
helper (or the context variant in
UpdateNodeGroupWithContext/GetClusterWithContext) so all 13 call sites remove
the repeated closure and propagate context where appropriate.
- Around line 108-121: GetClusterWithContext (and UpdateNodeGroupWithContext)
accept ctx but call c.ListClusters() inside the resolveID closure, so the
context isn't propagated; add a new method ListClustersWithContext(ctx
context.Context) that mirrors ListClusters but calls the underlying request with
context, then update the resolveID closures in GetClusterWithContext and
UpdateNodeGroupWithContext to call c.ListClustersWithContext(ctx) instead of
c.ListClusters() so cancellation and deadlines apply to the listing operation.

---

Nitpick comments:
In `@pkg/sdk/snapshot_test.go`:
- Around line 26-66: Update TestClientCreateSnapshot to be table-driven and
cover both identifier and name lookup: create a table of cases with input
strings {volumeIDStr, "test-volume"} and for each subtest call
client.CreateSnapshot(case.input, snapshotDescription). Reuse the existing
httptest server but ensure the GET /volumes handler returns the volume with Name
"test-volume" and ID volumeID so name-based lookups succeed; in each subtest
assert no error and that the returned snapshot.VolumeID equals the canonical
volumeID (expectedSnapshot.VolumeID) and other fields match. Keep the existing
request body assertions but compare req["volume_id"] to case.input so the test
verifies both ID-passthrough and name-passthrough behavior in CreateSnapshot.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1e19cc21-7d78-4121-b3ce-93ed7b77524a

📥 Commits

Reviewing files that changed from the base of the PR and between 5faabcc and 7b9c22e.

📒 Files selected for processing (11)
  • cmd/cloud/autoscaling_cli_test.go
  • cmd/cloud/dns.go
  • cmd/cloud/snapshot.go
  • internal/ccm/instances_test.go
  • pkg/sdk/autoscaling.go
  • pkg/sdk/compute.go
  • pkg/sdk/compute_test.go
  • pkg/sdk/dns.go
  • pkg/sdk/kubernetes.go
  • pkg/sdk/snapshot.go
  • pkg/sdk/snapshot_test.go

Comment thread pkg/sdk/compute.go
Comment thread pkg/sdk/snapshot_test.go Outdated
Comment thread pkg/sdk/snapshot.go Outdated
Copy link
Copy Markdown

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

poyrazK added 2 commits May 7, 2026 21:59
- Add ListVolumesWithContext(ctx context.Context) to volume.go
- Add resolveIDWithContext to client.go for context-aware ID resolution
- Update CreateSnapshot to take context.Context and use resolveIDWithContext
- Update GetInstanceWithContext and TerminateInstanceWithContext to use ListInstancesWithContext
- Update CLI snapshot create command to pass context.Context
- Update test calls to include context.Background()
Copilot AI review requested due to automatic review settings May 7, 2026 19:12
- Add resolveInstanceID/resolveInstanceIDWithContext helpers in compute.go
- Add resolveClusterID/resolveClusterIDWithContext helpers in kubernetes.go
- Add ListClustersWithContext for proper context propagation
- Update GetClusterWithContext and UpdateNodeGroupWithContext to use context-aware resolution
- Refactor 9 call sites in compute.go and 13 in kubernetes.go to use helpers
Copy link
Copy Markdown

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

poyrazK added 2 commits May 7, 2026 22:42
kubernetes.go - 11 new WithContext methods:
- DeleteClusterWithContext, GetKubeconfigWithContext, RepairClusterWithContext
- ScaleClusterWithContext, GetClusterHealthWithContext, UpgradeClusterWithContext
- RotateSecretsWithContext, CreateBackupWithContext, RestoreBackupWithContext
- AddNodeGroupWithContext, DeleteNodeGroupWithContext

compute.go - 6 new WithContext methods:
- GetConsoleURLWithContext, UpdateInstanceMetadataWithContext, StopInstanceWithContext
- GetInstanceLogsWithContext, ResizeInstanceWithContext, GetInstanceStatsWithContext
- Refactored GetInstance to delegate to GetInstanceWithContext

All WithContext methods use resolveIDWithContext for proper context propagation.
These non-context-aware helpers are no longer used since all call
sites now use the *WithContext variants for proper context support.
Copilot AI review requested due to automatic review settings May 7, 2026 19:49
Copy link
Copy Markdown

@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

🧹 Nitpick comments (2)
pkg/sdk/kubernetes.go (1)

166-174: ⚡ Quick win

Escape role before appending it to the kubeconfig query string.

Raw concatenation breaks valid role values containing reserved characters and can accidentally add extra query parameters. Build the query with url.Values or url.QueryEscape(role) before calling getWithContext.

Suggested fix
  path := clustersPath + "/" + id + "/kubeconfig"
  if role != "" {
-		path += "?role=" + role
+		path += "?role=" + url.QueryEscape(role)
  }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/sdk/kubernetes.go` around lines 166 - 174, In GetKubeconfigWithContext,
the code appends the raw role to the query string which breaks for reserved
characters; update the path construction to URL-encode role (e.g. use
url.QueryEscape(role)) or build the query with url.Values and encode it, then
append the encoded query to clustersPath+"/"+id+"/kubeconfig" before calling
getWithContext; reference the GetKubeconfigWithContext function and ensure
existing helpers like resolveClusterIDWithContext and clustersPath are used and
getWithContext receives a properly escaped query string.
pkg/sdk/snapshot_test.go (1)

27-71: ⚡ Quick win

Cover the new resolution path, not just the UUID fast-path.

This test still passes a full UUID, so CreateSnapshot returns before exercising the new /volumes lookup. Please add table-driven cases for exact name resolution, and for a unique short ID if that is part of the supported contract, so the feature added in this PR is actually protected.

As per coding guidelines, "Use table-driven tests in test files".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/sdk/snapshot_test.go` around lines 27 - 71, Update
TestClientCreateSnapshot to use a table-driven approach that covers the new
resolution paths (not just full UUID fast-path): create test cases for (1) full
UUID input (current behavior), (2) exact volume name resolution which should
trigger the /volumes GET lookup, and (3) a supported unique short ID case if
CreateSnapshot supports it; for each case exercise
NewClient(...).CreateSnapshot(ctx, input, snapshotDescription) and assert
expected behavior/requests (e.g., that the handler receives a /volumes GET for
name/short-id cases and that the returned snapshot matches expectedSnapshot);
keep references to TestClientCreateSnapshot, CreateSnapshot, NewClient,
snapshotPath and snapshotDescription so the logic locates the right assertions
and server handler branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cmd/cloud/snapshot.go`:
- Line 67: The call desc, _ := cmd.Flags().GetString("desc") silently ignores
errors; change it to capture and handle the error from
cmd.Flags().GetString("desc") (e.g., desc, err :=
cmd.Flags().GetString("desc")), check err and return it (or wrap with context)
so the CLI exits on flag retrieval failure; update the surrounding function (the
command handler using desc) to propagate the error instead of continuing with a
discarded error.

---

Nitpick comments:
In `@pkg/sdk/kubernetes.go`:
- Around line 166-174: In GetKubeconfigWithContext, the code appends the raw
role to the query string which breaks for reserved characters; update the path
construction to URL-encode role (e.g. use url.QueryEscape(role)) or build the
query with url.Values and encode it, then append the encoded query to
clustersPath+"/"+id+"/kubeconfig" before calling getWithContext; reference the
GetKubeconfigWithContext function and ensure existing helpers like
resolveClusterIDWithContext and clustersPath are used and getWithContext
receives a properly escaped query string.

In `@pkg/sdk/snapshot_test.go`:
- Around line 27-71: Update TestClientCreateSnapshot to use a table-driven
approach that covers the new resolution paths (not just full UUID fast-path):
create test cases for (1) full UUID input (current behavior), (2) exact volume
name resolution which should trigger the /volumes GET lookup, and (3) a
supported unique short ID case if CreateSnapshot supports it; for each case
exercise NewClient(...).CreateSnapshot(ctx, input, snapshotDescription) and
assert expected behavior/requests (e.g., that the handler receives a /volumes
GET for name/short-id cases and that the returned snapshot matches
expectedSnapshot); keep references to TestClientCreateSnapshot, CreateSnapshot,
NewClient, snapshotPath and snapshotDescription so the logic locates the right
assertions and server handler branches.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 70ffd198-f11a-4141-a9f3-7a305044dd62

📥 Commits

Reviewing files that changed from the base of the PR and between 7b9c22e and a626e9c.

📒 Files selected for processing (8)
  • cmd/cloud/snapshot.go
  • internal/csi/driver_test.go
  • pkg/sdk/client.go
  • pkg/sdk/compute.go
  • pkg/sdk/kubernetes.go
  • pkg/sdk/snapshot.go
  • pkg/sdk/snapshot_test.go
  • pkg/sdk/volume.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • pkg/sdk/snapshot.go
  • pkg/sdk/compute.go

Comment thread cmd/cloud/snapshot.go Outdated
- snapshot.go: Properly handle error from cmd.Flags().GetString("desc")
- kubernetes.go: URL-encode role parameter in GetKubeconfigWithContext
Copy link
Copy Markdown

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Owner Author

@poyrazK poyrazK left a comment

Choose a reason for hiding this comment

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

It's okay to merge

@poyrazK poyrazK merged commit e8c391f into main May 7, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants