Refactor generate-k8s-api in Makefile #24651
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mhofstetter
added
kind/cleanup
mhofstetter
requested review from
sayboras,
nathanjsweet,
gandro,
bimmlerd,
aanm and
christarazi
joestringer
reviewed
Mar 30, 2023
Makefile
Outdated
| @@ -56,48 +56,50 @@ TEST_LDFLAGS=-ldflags "-X github.com/cilium/cilium/pkg/kvstore.consulDummyAddres | |||
| TEST_UNITTEST_LDFLAGS=-ldflags "-X github.com/cilium/cilium/pkg/datapath.DatapathSHA256=e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" | |||
|
|
|||
| define generate_k8s_api | |||
| cd "./vendor/k8s.io/code-generator" && \ | |||
| GO111MODULE=off bash ./generate-groups.sh $(1) \ | |||
| $(QUIET) cd "./vendor/k8s.io/code-generator" && \ | |||
There was a problem hiding this comment.
I think that we end up potentially duplicating these $(QUIET) statements if we have them inside the templates like this. For instance
$(QUIET) $(call generate_k8s_api_deepcopy_deepequal_client,client,github.com/cilium/cilium/pkg/k8s/slim/k8s/api,"$\ ...
=>
$(call generate_k8s_api,deepcopy,github.com/cilium/cilium/pkg/k8s/client,$(1),$(2),$(3))
=>
$(QUIET) cd "./vendor/k8s.io/code-generator" && \ ...
Maybe we should only use those statements in the main targets and not inside the templates?
There was a problem hiding this comment.
hey @joestringer , thanks for your input!
over the whole PR - they ended up being only in the template - and not in both (main target and template) 😄
but would be definitely nicer to have them only in the main targets. it's just that this only works as intended if the template only contains one command. otherwise (e.g. generate_k8s_protobuf (where tools are getting installed first)) only the first commands gets silenced.
-> i moved the $(QUIET)'s to the main target and chained the installation commands with the generation command in the mentioned case!
K8s generators behave quite differently when building within or outside of the GOPATH. Before these changes, when building outside of GOPATH, the generated artifacts have been generated within the directory 'github.com/cilium/cilium' within the working directory itself. With this commit, the artifacts are built into a temporary directory and copied into the right place. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Before this commit, the generator has been triggered for every package with the necessary types. This commit changes this, by generating per type (proto, deepcopy, deepequal, client, ...) which eases the use of understanding, optimizes performance in case of building outside of GOPATH and allows better reuse of generated deepequal methods from other types. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
By generating deepequal files for all packages at once - an issue arised around reusing the deepequal methods of referenced types. Depending on the order and building in-/ouside of GOPATH, the generated artifacts differed. Upgrading to a new version of github.com/cilium/deepequal-gen fixes the issue and adds the missing reusages. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit removes the hardcoded list of deepequal & deepcopy go packages. Instead the relevant packages are getting evaluated by looking up packages which contain go files with the corresponding markers. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit enforces deletion of generated k8s-client & protobuf artifacts before generating and checking for diffs in 'check-k8s-code-gen.sh'. This helps in detecting issues and stale artifacts. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
make generate-k8s-api no longer relies on the GOPATH. Therefore, it can be moved when executing the checks in the GH action. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
mhofstetter
force-pushed
the
pr/mhofstetter/generate-k8s-api-worktree
branch
from
c95dcad
to
06a366b
Compare
1 task
aanm
approved these changes
Mar 31, 2023
|
/test |
bimmlerd
approved these changes
Mar 31, 2023
nathanjsweet
approved these changes
Mar 31, 2023
christarazi
added
backport/author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
make generate-k8s-apioutside ofGOPATHby generating into tmp directory