Refactor capdctl #84
Refactor capdctl #84
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XXL
|
/cc @vincepri |
|
part of #23 |
There was a problem hiding this comment.
A few things need cleanup and license files need to be in all new files. Besides that my only other point of discussion: Do you think these files referencing CAPI types could live in like a /v1alpha1/ directory so we know which ones to use for v1a1 and which ones to use for v1a2? Or do you think there's a better solution to that problem? I think that's out of scope for this PR but I'm curious about your thoughts since I'd like to be able to support both (and future) versions of CAPI
cmd/capdctl/main.go
Outdated
| "k8s.io/client-go/restmapper" | ||
| "sigs.k8s.io/cluster-api-provider-docker/kind/controlplane" | ||
| "sigs.k8s.io/cluster-api-provider-docker/objects" | ||
| _ "sigs.k8s.io/cluster-api-provider-docker/objects" |
There was a problem hiding this comment.
imported twice but once with an underscore?
| @@ -0,0 +1,41 @@ | |||
| package controlplane | |||
There was a problem hiding this comment.
this and all new files are missing license header
| ) | ||
|
|
||
| // getCRDs should actually use kustomize to correctly build the manager yaml. | ||
| // HACK: this is a hacked function |
objects/capi
Outdated
| @@ -0,0 +1,70 @@ | |||
| package objects | |||
There was a problem hiding this comment.
This file should be removed
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
It doesn't uh, actually work right now seemingly 😅 |
| tgz := tar.NewReader(gz) | ||
| var buf bytes.Buffer | ||
|
|
||
| for { |
There was a problem hiding this comment.
Can we run tar -xvf using cmd.exec instead? That, I think, will greatly simplify this code.
There was a problem hiding this comment.
This was just moved, I created it in a previous PR. I'm not execing for the same reason @liztio is removing the exec to kubectl. It is easier to manage dependencies through go.mod. In both cases it is very possible. If we were to exec we'd have to first make sure the right version of kubectl / tar /whatever are installed. It adds unnecessary complication for the end user when we can do it in Go and be done with it.
There was a problem hiding this comment.
That's a valid reason that applies more to kubectl than utils like tar or any other linux utils we may want to use.
There was a problem hiding this comment.
we'd still need to walk the entire directory and read these files in. And we don't want to assume tar exists with the same arguments on every system we need to interact with. cmd.Exec() should always be a last resort IMO
There was a problem hiding this comment.
I think this is avoidable complexity. But, I am OK with doing this 😄
| limitations under the License. | ||
| */ | ||
|
|
||
| package objects |
There was a problem hiding this comment.
suggest calling this package manifests. objects is a little confusing
There was a problem hiding this comment.
that's what Kubernetes calls them 🤷♀️
|
/test pull-cluster-api-provider-docker-verify |
|
After the last two PRs we uncovered an issue with the examples. This now works! |
|
/approve We worked on this together for a long time which is why I am both approving and lgtm |
k8s-ci-robot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chuckha, liztio 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 this PR does / why we need it:
Removes all gnarly
kubectl execlines we have lying around incapdctlWhich issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
😅
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
Release note: