Skip to content

Use the modern API to import clusters#184

Open
moio wants to merge 1 commit into
rancher:mainfrom
moio:update-imported-api
Open

Use the modern API to import clusters#184
moio wants to merge 1 commit into
rancher:mainfrom
moio:update-imported-api

Conversation

@moio

@moio moio commented May 18, 2026

Copy link
Copy Markdown
Contributor

This creates management cluster objects from the Kubernetes API instead of driving browser-like usage of Norman API to create provisioning clusters.

This also generalizes a bit the kubectl.Wait and Get methods to support the new workflow.

This creates management cluster objects from the Kubernetes API instead of driving browser-like usage of Norman API to create provisioning clusters.

This also generalizes a bit the kubectl.Wait and Get methods to support the new workflow.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the downstream-cluster import workflow to create Rancher management Cluster objects via the Kubernetes API (kubectl) rather than simulating UI/Norman API calls, and adjusts the kubectl helpers to support the new flow.

Changes:

  • Removed k6-based Norman/API “imported cluster” creation and its env-driven loop.
  • Generalized kubectl wait helper to accept arbitrary --for conditions and updated call sites accordingly.
  • Added a kubectl Get convenience for raw string output and added a new kubectl create -f path to create management clusters.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
k6/rancher/rancher_utils.js Removes the old Norman/UI-like imported cluster creation helper.
k6/rancher/rancher_setup.js Drops imported-cluster creation loop/env var usage; keeps only login/logout.
internal/kubectl/kubectl.go Adds Create, generalizes WaitFor... to WaitFor, and extends Get to support *string outputs.
cmd/dartboard/subcommands/deploy.go Switches cluster import bootstrap to creating management clusters via kubectl and updates wait/get logic to match.
Comments suppressed due to low confidence (2)

cmd/dartboard/subcommands/deploy.go:709

  • The error wrapping here uses %e, which is for floats; this will render as %!e(...) and also won't wrap the underlying error. Use %w (or %v if not wrapping) and consider updating the message to match the operation (kubectl create vs "applying").
	if err := kubectl.Create(upstream.Kubeconfig, yamlFile.Name()); err != nil {
		return fmt.Errorf("applying imported clusters YAML: %e", err)
	}

cmd/dartboard/subcommands/deploy.go:733

  • clusterID parsing is fragile: kubectl get -o jsonpath can return empty output (no match) or multiple/whitespace-separated results. strings.Split(*out, " ")[0] can yield an empty ID; trim/field-split the output and explicitly error if no cluster ID is found (or retry until the created Cluster appears).
	jsonPath := fmt.Sprintf(".items[?(@.spec.displayName==\"%s\")].metadata.name", name)

	out := new(string)
	if err = kubectl.Get(upstream.Kubeconfig, "clusters.management.cattle.io", "", "", jsonPath, out); err != nil {
		return
	}

	clusterID = strings.Split(*out, " ")[0]

	err = kubectl.WaitFor(upstream.Kubeconfig, "clusterregistrationtokens.management.cattle.io", "default-token", clusterID, "create", 1)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

return Exec(kubePath, log.Writer(), "apply", "-f", filePath)
}

func Create(kubePath string, filePath string) interface{} {
yamlFile, err := os.CreateTemp("", "dartboard-imported-clusters-*.yaml")
if err != nil {
return fmt.Errorf("creating temp file for imported clusters: %w", err)
}

@git-ival git-ival left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There has been a large change to how we register clusters which requires rebasing, conflicts will need to be resolved before this PR can progress 😬.

Other than that, I'm not sure we even need to keep the rancher_setup.js file at all.

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