🌱 Use upstream cluster RESTConfig utility #248
Conversation
k8s-ci-robot
added
cncf-cla: yes
jimmidyson
force-pushed
the
jimmi/simplify-restconfig
branch
2 times, most recently
from
64fada5
to
745148d
Compare
controllers/controllers_test.go
Outdated
| Namespace: cluster.Namespace, | ||
| }, | ||
| Data: map[string][]byte{ | ||
| secret.KubeconfigDataName: []byte(`apiVersion: v1 |
There was a problem hiding this comment.
Nit: maybe we can move this into a var declaration outside the function so the indentation looks better like this:
var config = []byte(`apiVersion: v1
kind: Config
clusters:
- cluster:
server: http://127.0.0.1:8080
name: ` + cluster.Name + `
contexts:
- context:
cluster: ` + cluster.Name + `
name: ` + cluster.Name + `
current-context: ` + cluster.Name + `
`),
},
}
}
There was a problem hiding this comment.
Hard to move it outside as a var because it includes cluster.Name from the cluster argument. I've added new line at the start so indentation looks better but kept it in the function. Hope that's ok.
This commit simplifies the codebase by removing custom kubeconfig code and instead using the existing fucntionality from cluster-api project.
jimmidyson
force-pushed
the
jimmi/simplify-restconfig
branch
from
745148d
to
db42942
Compare
|
@jimmidyson: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Since the beginning of time (well a long time anyway 😆 ) https://github.com/kubernetes-sigs/cluster-api/blame/main/controllers/remote/cluster.go#L52 |
| @@ -117,6 +141,8 @@ var _ = Describe("Testing HelmChartProxy and HelmReleaseProxy reconcile", func() | |||
| cluster := cluster1.DeepCopy() | |||
| err := k8sClient.Create(ctx, cluster) | |||
| Expect(err).ToNot(HaveOccurred()) | |||
| err = k8sClient.Create(ctx, newKubeconfigSecretForCluster(cluster)) | |||
There was a problem hiding this comment.
| err = k8sClient.Create(ctx, newKubeconfigSecretForCluster(cluster)) | |
| Expect(k8sClient.Create(ctx, newKubeconfigSecretForCluster(cluster))).To(Succeed()) |
I think this style is more readable, but your change is parallel with the rest of the tests, so 🤷🏻.
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jimmidyson, mboersma The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
**What problem does this PR solve?**: Upgrade CAAPH to 0.2.4 This release includes fixes needed for supporting CAAPH in airgap environment. 🌱 Use upstream cluster RESTConfig utility by @jimmidyson in kubernetes-sigs/cluster-api-addon-provider-helm#248 🐛 Fix OCI client configuration logic by @jimmidyson in kubernetes-sigs/cluster-api-addon-provider-helm#252 🐛 pointer checks if user doesn't specify CASecret by @faiq in kubernetes-sigs/cluster-api-addon-provider-helm#253 **Which issue(s) this PR fixes**: Fixes # **How Has This Been Tested?**: <!-- Please describe the tests that you ran to verify your changes. Provide output from the tests and any manual steps needed to replicate the tests. --> **Special notes for your reviewer**: <!-- Use this to provide any additional information to the reviewers. This may include: - Best way to review the PR. - Where the author wants the most review attention on. - etc. -->
This commit simplifies the codebase by removing custom kubeconfig code
and instead using the existing fucntionality from cluster-api project.