🐛 Refactor alpha config-gen to use embed from go 1.16
#2204
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pwittrock 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
approved
pwittrock
changed the title
alpha config-gen to use embed from go 1.16alpha config-gen to use embed from go 1.16
pwittrock
force-pushed
the
config-gen
branch
3 times, most recently
from
20ae9bc to
5e91c7d
Compare
|
Looks good to me. Happy to see extracting logic into a function over baking it into the Cobra command. |
pkg/cli/alpha/config-gen/cmd.go
Outdated
| &framework.ResourcePatchTemplate{ | ||
| Selector: &framework.Selector{ | ||
| Kinds: []string{"Deployment"}, | ||
| Names: []string{"controller-manager"}, |
There was a problem hiding this comment.
This used to also have a namespace selector. Is that no longer needed? Since this is no longer in a closure itself, you'd need to use a template string.
| @@ -31,35 +33,33 @@ type API struct { | |||
| // Advanced function for transforming kubebuilder output by looking at the resources and modifying them. | |||
| func main() { | |||
| api := &API{} | |||
| c := framework.TemplateProcessor{ | |||
There was a problem hiding this comment.
It looks like this one doesn't actually use any templates. Unless I'm missing something, it could use SimpleProcessor instead: https://pkg.go.dev/sigs.k8s.io/kustomize/kyaml/fn/framework#SimpleProcessor
|
|
||
| import ( | ||
| _ "sigs.k8s.io/controller-runtime" | ||
| _ "sigs.k8s.io/controller-runtime/pkg/scheme" |
There was a problem hiding this comment.
Why does the framework upgrade make this necessary?
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/override |
|
@pwittrock: /override requires a failed status context to operate on, but none was given In response to this:
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/test-infra repository. |
|
/override APIDiff |
|
@pwittrock: /override requires a failed status context to operate on.
Only the following contexts were expected:
In response to this:
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/test-infra repository. |
|
/override tide |
|
/override Verify API differences |
|
@DirectXMan12: /override requires a failed status context to operate on.
Only the following contexts were expected:
In response to this:
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/test-infra repository. |
|
@pwittrock: Overrode contexts on behalf of pwittrock: tide In response to this:
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/test-infra repository. |
|
Seems like prow is not merging this. Any concerns with me manually merging? |
|
/override Verify API differences |
|
@pwittrock: /override requires a failed status context to operate on.
Only the following contexts were expected:
In response to this:
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/test-infra repository. |
Update the version of kyaml to use the latest framework. This switches to embed (go 1.16) from pkgr for embedding template files into the binary.
No testdata should change as part of this PR to ensure it is a pure refactoring.