Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(build): introduces controller-gen scaffolding #121

Conversation

eoinfennessy
Copy link
Member

@eoinfennessy eoinfennessy commented Jan 7, 2025

These changes introduce the controller-gen tool and a make target to generate CRDs and DeepCopy methods. This target is added as a dependency of the build target, and runs only when changes are made to source files of the generated CRDs and methods.

Placeholder types for the MeshFederation kind have been added to verify generation.

Copy link

openshift-ci bot commented Jan 7, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the size/L label Jan 7, 2025
Copy link
Collaborator

@jewertow jewertow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you create MeshFederation resource? I was trying to add a resource with the following command:

kubebuilder create api --group federation --version v1alpha1 --kind ImportedService

but I got this error:

FATA failed to create API: unable to run pre-scaffold tasks of "base.go.kubebuilder.io/v4": cmd/main.go file should present in the root directory

Are you going to change cmd package in this PR?

@eoinfennessy
Copy link
Member Author

How did you create MeshFederation resource?

I scaffolded a new project and API using kubebuilder, then copied the files in api/v1alpha1 and pasted them into their current location.

Are you going to change cmd package in this PR?

I wasn't intending to. I think enabling us to use controller-gen to generate code and CRDs is enough for this PR. The next step (a follow-up PR) will set up a simple controller for the MeshFederation resource. That work could include changing the project structure so that more APIs can be scaffolded using kubebuilder, but in my opinion this is not that important as creating each API is a one-time operation that can easily be achieved with a little copy-pasting, similar to what was done here.

@jewertow
Copy link
Collaborator

jewertow commented Jan 9, 2025

Ok, that works for me.

@eoinfennessy eoinfennessy force-pushed the introduce-kubebuilder branch from 44bd8a2 to bc1d832 Compare January 9, 2025 13:58
@eoinfennessy eoinfennessy changed the title WIP: Introduce kubebuilder Introduce controller-gen Jan 10, 2025
@eoinfennessy eoinfennessy marked this pull request as ready for review January 10, 2025 11:20
@eoinfennessy eoinfennessy force-pushed the introduce-kubebuilder branch from f353806 to 24ec1d5 Compare January 10, 2025 12:25
Copy link
Collaborator

@bartoszmajsak bartoszmajsak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can simplify the entire generation to one command. You can bundle my suggestions to a single commit.

@@ -36,7 +47,7 @@ deps: ## Downloads required dependencies

EXTRA_BUILD_ARGS?=
.PHONY: build
build: deps $(PROTOBUF_GEN) ## Builds the project
build: deps $(PROTOBUF_GEN) $(DEEP_COPY_GEN) $(CRD_GEN) ## Builds the project
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
build: deps $(PROTOBUF_GEN) $(DEEP_COPY_GEN) $(CRD_GEN) ## Builds the project
build: deps $(PROTOBUF_GEN) $(CRD_GEN) ## Builds the project

Makefile Outdated
Comment on lines 24 to 31
DEEP_COPY_DIR := $(PROJECT_DIR)/api/v1alpha1
DEEP_COPY_SRC := $(shell find $(DEEP_COPY_DIR) -type f -not -name "*deepcopy.go")
DEEP_COPY_GEN := $(DEEP_COPY_DIR)/zz_generated.deepcopy.go

CRD_SRC_DIR := $(PROJECT_DIR)/api/v1alpha1
CRD_SRC := $(shell find $(CRD_SRC_DIR) -type f -name "*.go")
CRD_GEN_DIR := $(PROJECT_DIR)/chart/crds
CRD_GEN := $(shell find $(CRD_GEN_DIR) -type f -name "*.yaml")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DEEP_COPY_DIR := $(PROJECT_DIR)/api/v1alpha1
DEEP_COPY_SRC := $(shell find $(DEEP_COPY_DIR) -type f -not -name "*deepcopy.go")
DEEP_COPY_GEN := $(DEEP_COPY_DIR)/zz_generated.deepcopy.go
CRD_SRC_DIR := $(PROJECT_DIR)/api/v1alpha1
CRD_SRC := $(shell find $(CRD_SRC_DIR) -type f -name "*.go")
CRD_GEN_DIR := $(PROJECT_DIR)/chart/crds
CRD_GEN := $(shell find $(CRD_GEN_DIR) -type f -name "*.yaml")
CRD_SRC_DIR := $(PROJECT_DIR)/api/v1alpha1
CRD_SRC := $(shell find $(CRD_SRC_DIR) -type f -name "*.go")
CRD_GEN_DIR := $(PROJECT_DIR)/chart/crds
CRD_GEN := $(shell find $(CRD_GEN_DIR) -type f -name "*.yaml")

Makefile Outdated
Comment on lines 149 to 154
$(DEEP_COPY_GEN): $(DEEP_COPY_SRC) ## Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations.
$(CONTROLLER_GEN) object:headerFile=$(LICENSE_FILE) paths="./..."

$(CRD_GEN): $(CRD_SRC) ## Generate CustomResourceDefinition objects.
$(CONTROLLER_GEN) crd paths="./..." output:crd:artifacts:config=chart/crds

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving forward we will also add webhooks, so having the entire generation as one command will be more efficient.

What we missed here is dependency to the controller-gen itself - if it's not present in the path it will now be automatically fetched.

Suggested change
$(DEEP_COPY_GEN): $(DEEP_COPY_SRC) ## Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations.
$(CONTROLLER_GEN) object:headerFile=$(LICENSE_FILE) paths="./..."
$(CRD_GEN): $(CRD_SRC) ## Generate CustomResourceDefinition objects.
$(CONTROLLER_GEN) crd paths="./..." output:crd:artifacts:config=chart/crds
$(CRD_GEN): $(CRD_SRC) $(CONTROLLER_GEN) ## Generates CRDs and other controller-runtime scaffolding.
$(CONTROLLER_GEN) paths="$(CRD_SRC_DIR)/..." \
crd output:crd:artifacts:config="$(CRD_GEN_DIR)" \
object:headerFile="$(LICENSE_FILE)"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's more to that btw (RBAC generation, kube-builder directives in controllers etc), but we can fine-tune it as we go.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other thing from the top of my head - if someone opens PR with code changes but forgets to regenerate CRDs and other stuff we will end up with misaligned artifacts, i.e. YAMLs will be behind. I will capture it as an issue.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@bartoszmajsak bartoszmajsak changed the title Introduce controller-gen feat(build): introduces controller-gen scaffolding Jan 10, 2025
@jewertow
Copy link
Collaborator

/retest

@openshift-merge-bot openshift-merge-bot bot merged commit b0ea4ff into openshift-service-mesh:master Jan 10, 2025
4 checks passed
bartoszmajsak added a commit to bartoszmajsak/ossm-federation that referenced this pull request Jan 10, 2025
openshift-merge-bot bot pushed a commit that referenced this pull request Jan 13, 2025
@bartoszmajsak
Copy link
Collaborator

Part of #148

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants