Skip to content

fix: node-pool delete keeps labels/taints on other pools; no panic on --region without API key - #614

Merged
fulviodenza merged 3 commits into
masterfrom
fix/nodepool-delete-followups
Nov 3, 2026
Merged

fulviodenza merged 3 commits into
masterfrom
fix/nodepool-delete-followups

Conversation

@fulviodenza

Copy link
Copy Markdown
Contributor

Follow-up to #607, fixing two issues found while reviewing it.

1. node-pool delete wiped labels and taints on the remaining pools

civo k8s node-pool delete removed pools by rebuilding the cluster's pool list with only id, count and size, then sending it through UpdateKubernetesCluster. When the list is shorter than the cluster's, the API replaces the pools with the request (SetPools(updateReq.Pools) in api-go UpdateCluster), so every remaining pool lost its labels, and taints went out as null.

Now the command:

  • resolves every requested ID (or prefix) against the cluster's pools first, and fails before touching anything if an ID matches nothing, the same pool is given twice, or the request would remove every pool (the API refuses to delete the last pool, so this used to be able to fail halfway);
  • deletes each pool with DeleteKubernetesClusterPool (DELETE /v2/kubernetes/clusters/{id}/pools/{pool_id}), which only removes that pool and releases its quota;
  • reports which pools were deleted if a later deletion fails.

It also drops the unreachable len(args) == 1 branch (which indexed args[1]) and the package-level slice that accumulated across invocations. Matching still goes through utility.RemoveNodePool, so prefix behaviour is unchanged from #607.

2. --region with no API key panicked

Every command set client.Region from --region before checking the error from config.CivoAPIClient(), which returns a nil client when no API key is configured:

$ HOME=$(mktemp -d) civo volume ls --region LON1
Error: Creating the connection to Civo's API failed because no API key is supplied. ...
panic: runtime error: invalid memory address or nil pointer dereference

The second commit moves the error check above the region override in all 119 call sites. It is a pure reordering (360 lines moved, nothing else changed), done with a pattern match and then checked so that no client.Region = common.RegionSet comes before an if err != nil anywhere in cmd/. With the fix the same command prints the normal "no API key" error.

Testing

  • New unit tests for ID resolution (prefix, unknown, duplicate, deleting every pool) and for the per-pool deletion loop, including stopping at the first failure.
  • go build, go vet, go test ./... and golangci-lint run all pass locally.
  • Reproduced the panic on master and confirmed the fix with a throwaway HOME.
  • Not run against the live API: the node-pool deletion should be tried on a real cluster with two or more labelled/tainted pools before release.

`civo k8s node-pool delete` removed pools by sending the cluster's pool
list back through UpdateKubernetesCluster with only ID, count and size.
When that list is shorter than the cluster's, the API replaces the pools
with it, so every remaining pool lost its labels and taints.

Resolve the requested IDs up front (failing on unknown IDs, duplicates,
or a request that would remove every pool, before touching anything),
then delete each pool with DeleteKubernetesClusterPool, which leaves the
other pools untouched and releases quota for the deleted pool.

Also drops the unreachable len(args) == 1 branch, which indexed args[1],
and the package-level slice that accumulated across invocations.
Every command created the client and then set client.Region from
--region before checking the error from config.CivoAPIClient(). With no
API key configured CivoAPIClient returns a nil client, so any command run
with --region panicked with a nil pointer dereference instead of printing
the 'no API key' error.

Move the error check above the region override in all 119 call sites.
This is a pure reordering, no other changes.
@fulviodenza
fulviodenza merged commit 9baa984 into master Nov 3, 2026
3 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

Development

Successfully merging this pull request may close these issues.

2 participants