Use go embed and remove go-bindata dependency #15834
Conversation
aanm
added
kind/cleanup
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
aanm
added
the
release-note/misc
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
|
|
In Golang 1.16 we can use go embed to embed files without using a 3rd party library. Thus we can remove the go-bindata from our list of dependencies. Signed-off-by: André Martins <andre@cilium.io>
aanm
force-pushed
the
pr/go-embed
branch
2 times, most recently
from
37ef971
to
d4b2e94
Compare
There was a problem hiding this comment.
🚀 There are other references [1] of go-bindata in Makefile that need to be removed as well?
[1] https://github.com/cilium/cilium/blob/master/Makefile#L434
@aditighag it's already gone 🧙 |
|
test-me-please |
pkg/k8s/cnp.go
Outdated
| @@ -78,7 +78,7 @@ func (c *CNPStatusUpdateContext) updateStatus(ctx context.Context, cnp *types.Sl | |||
| if option.Config.K8sEventHandover { | |||
| err = c.updateViaKVStore(ctx, cnp, false, false, policyImportErr, rev, cnp.Annotations) | |||
| } else { | |||
| err = c.updateViaAPIServer(cnp, false, false, policyImportErr, rev, cnp.Annotations) | |||
| err = c.UpdateViaAPIServer(cnp, false, false, policyImportErr, rev, cnp.Annotations) | |||
There was a problem hiding this comment.
Why export the function if it's only used in unit tests?
There was a problem hiding this comment.
It's in the commit message. I had to the unit tests a dedicated package to avoid import cycle
There was a problem hiding this comment.
Why is there an import cycle? Is this the only way to avoid it?
There was a problem hiding this comment.
That's not what I was referring to. I meant to ask if you tried doing something like this to avoid exporting a method for unit tests only - https://medium.com/@robiplus/golang-trick-export-for-test-aa16cbd7b8cd.
There was a problem hiding this comment.
@christarazi the unit tests (pkg/k8s) imports pkg/k8s/apis/cilium.io/v2/client which imports pkg/k8s
There was a problem hiding this comment.
@aditighag Maybe I'm missing something but I don't see how that can work since the link referes to a package function where this is a structure's method?
There was a problem hiding this comment.
I think you'd need to have a test file specifically be a part of the package k8s which does var UpdateViaAPIServer = updateViaAPIServer which then you use in the package k8s_test. However, that's moot because of the import cycle problem.
Do you think that we should refactor so we avoid the import cycle to begin with? I feel that when we run into an import cycle, that usually means we should reorg the code a bit, no?
There was a problem hiding this comment.
@christarazi but that won't work because updateViaAPIServer is a method, not a function, like in the example from the medium link.
Do you think that we should refactor so we avoid the import cycle to begin with? I feel that when we run into an import cycle, that usually means we should reorg the code a bit, no?
I would say so but the import cycle happens in the testing package. I only found it when the CI complained as far as "production" code, it's not affected. I thought about on having a benchmark package inside pkg/k8s dedicated to this _test.go file but in the end it would be the same as simply renaming package k8s to package k8s_test, which is what the PR is doing.
There was a problem hiding this comment.
@aanm Sure, the trick works - aditighag@aec66c0.
|
The entire CI was green, I only added a new change suggested by Aditi so there's no need to run the entire CI again. |
The CRDs should be self-contained in a directory with different versions. By also moving the registration of the CRDs into a self-contained package, the operator, the entity that registers the CRDs since Cilium 1.9, will be the only binary that will load these files into memory, reducing the memory consumption of the cilium-agent. The test cnp_test.go had to be put in a dedicated package to avoid import cycle. Signed-off-by: André Martins <andre@cilium.io>
Read per commit basis