Allow importing kustomize API's without relying on plugins
#5525
Conversation
Introduce `kustomize_disable_go_plugin_support` go build tag to decouple the kustomize API from the `plugins` package dependency. This is advantageous for applications embedding the kusstomize API without the need for dynamic Go plugins, mitigating an increase in binary size associated with the inclusion of the plugins dependency and the population of ELF sections like `.dynsym` and `.dynstr`. The flag provides applications with the flexibility to exclude the import, catering to scenarios where dynamic Go plugin support is unnecessary. Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @tigrato! |
|
Hi @tigrato. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
needs-ok-to-test
|
This PR has multiple commits, and the default merge method is: merge. 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. |
There was a problem hiding this comment.
Hi there, @tigrato! 👋
Thank you very much for your contribution!
Left a few nits, please let me know if anything is unclear.
|
/ok-to-test /label tide/merge-method-squash |
k8s-ci-robot
added
tide/merge-method-squash
|
I incorporated all the suggestions. |
k8s-ci-robot
added
the
lgtm
There was a problem hiding this comment.
Changes look good to me.
Thanks for the PR @tigrato!
/lgtm
/approve
| // but the loaded .so files are in shared memory, so one will get | ||
| // "this plugin already loaded" errors if the registry is maintained | ||
| // as a Loader instance variable. So make it a package variable. | ||
| var registry = make(map[string]resmap.Configurable) //nolint:gochecknoglobals |
There was a problem hiding this comment.
Probably a follow up, doesn't necessarily concern this PR as it was like this before - but it would be nice to pass the registry with the loader instead of having it as a global variable (probably like an optional builder pattern) to enable having restricted access to the map.
There was a problem hiding this comment.
I think the reason why this is a global variable is that because go panics if you try to load the same plugin twice.
If we do it per registry, one might end up with panics because loading twice isn't allowed in go plugin module although the plugin was only loaded once per registry's
The only viable way is to use a global registry
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tigrato, varshaprasad96 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
…tes-sigs#5525) * Allow importing kustomize API's without relying on `plugins` Introduce `kustomize_disable_go_plugin_support` go build tag to decouple the kustomize API from the `plugins` package dependency. This is advantageous for applications embedding the kusstomize API without the need for dynamic Go plugins, mitigating an increase in binary size associated with the inclusion of the plugins dependency and the population of ELF sections like `.dynsym` and `.dynstr`. The flag provides applications with the flexibility to exclude the import, catering to scenarios where dynamic Go plugin support is unnecessary. Signed-off-by: Tiago Silva <tiago.silva@goteleport.com> * fix golint by disabling some lint checks * handle code review suggestions --------- Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>
When kubernetes-sigs#5525 merged, it referenced `errors.New` function but that function doesn't exist. This PR replaces the call with simple `fmt.Errorf`.
* fix: use fmt.Errorf ubstead if non-exising `errors.New` When #5525 merged, it referenced `errors.New` function but that function doesn't exist. This PR replaces the call with simple `fmt.Errorf`. * Add lint check with kustomize_disable_go_plugin_support * move lint-api-static to /api/Makefile * clean golangci cache
Introduce
kustomize_disable_go_plugin_supportgo build tag to decouple the kustomize API from thepluginspackage dependency. This is advantageous for applications embedding the kustomize API without the need for dynamic Go plugins, mitigating an increase in binary size associated with the inclusion of the plugins dependency and the population of ELF sections like.dynsymand.dynstr.The flag provides applications with the flexibility to exclude the import, catering to scenarios where dynamic Go plugin support is unnecessary.
Fixes #5524