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

controller: adds yaml to install and deploy controller #106

Merged
merged 1 commit into from
Jan 31, 2022

Conversation

yati1998
Copy link
Contributor

this commit adds raw yaml generated using kustomize
in deploy/crds.yaml which can be used by others to
directly deploy the controller

Fixes: #105

Signed-off-by: yati1998 ypadia@redhat.com

@yati1998
Copy link
Contributor Author

@nixpanic @Rakshith-R please do add your reviews. Thanks, Rakshith for helping out with makefile.

Rakshith-R
Rakshith-R previously approved these changes Jan 28, 2022
Makefile Outdated
Comment on lines 86 to 89
manifests: controller-gen kustomize## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects.
$(CONTROLLER_GEN) rbac:roleName=manager-role crd webhook paths="{./api/...,./cmd/...,./controllers/...,./sidecar/...}" output:crd:artifacts:config=config/crd/bases
cd config/manager && $(KUSTOMIZE) edit set image controller=${CONTROLLER_IMG} $(KUSTOMIZE_RBAC_PROXY)
$(KUSTOMIZE) build config/default > deploy/controller/setup-controller.yaml
Copy link
Member

Choose a reason for hiding this comment

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

With this, yaml will be auto-generated whenever manifests is run and can be verified in the ci https://github.com/csi-addons/kubernetes-csi-addons/runs/4977203144?check_suite_focus=true#step:4:10

@Rakshith-R Rakshith-R requested a review from Madhu-1 January 28, 2022 07:06
$(CONTROLLER_GEN) rbac:roleName=manager-role crd webhook paths="{./api/...,./cmd/...,./controllers/...,./sidecar/...}" output:crd:artifacts:config=config/crd/bases
cd config/manager && $(KUSTOMIZE) edit set image controller=${CONTROLLER_IMG} $(KUSTOMIZE_RBAC_PROXY)
$(KUSTOMIZE) build config/default > deploy/controller/setup-controller.yaml
Copy link
Member

Choose a reason for hiding this comment

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

we should do kubectl dry-run to verify the template's syntax is correct.

@@ -0,0 +1,1050 @@
apiVersion: v1
Copy link
Member

Choose a reason for hiding this comment

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

is it possible to have 3 files, one for CRD, one for RBAC, and one for deployment? 1050 lines in a single file seem heavy for me.

Copy link
Contributor Author

@yati1998 yati1998 Jan 28, 2022

Choose a reason for hiding this comment

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

Yeah, we can, but here we are just building the config/default which generates all these at a place. I would prefer to have it as a TODO as it requires more modifications.

- --leader-elect
command:
- /manager
image: quay.io/csiaddons/k8s-controller:latest
Copy link
Member

Choose a reason for hiding this comment

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

hope this will get replaces when we do a release and release branch will contain the proper tagged image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, its fine we do edit this in makefile. I created the file with this image. We can update it with latest build.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably become an artifact when we do a release. Creating a tag in git can then generate the yaml, and post it as part of the release (somehow). It requires adding one or more jobs to the build-push workflow we already have.

@yati1998
Copy link
Contributor Author

@Madhu-1 @nixpanic @Rakshith-R Let's go with one file for now. This file is auto-generated via Kustomize and splitting it breaks some or the other things. So I guess we can add a TODO in Makefile itself so that we can have the rook implementation done. Please do share your thoughts on this and will update the PR accordingly.

Copy link
Collaborator

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

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

nit about the makefile comment after kustomize, looks good to me otherwise

Makefile Outdated
@@ -83,8 +83,10 @@ help: ## Display this help.
##@ Development

.PHONY: manifests
manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects.
manifests: controller-gen kustomize## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects.
Copy link
Collaborator

Choose a reason for hiding this comment

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

use a space between kustomize##, not sure if this does what is intended

- --leader-elect
command:
- /manager
image: quay.io/csiaddons/k8s-controller:latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably become an artifact when we do a release. Creating a tag in git can then generate the yaml, and post it as part of the release (somehow). It requires adding one or more jobs to the build-push workflow we already have.

this commit adds raw yaml generated using kustomize
in deploy/controller/ which can be used by others to
directly deploy the controller

Fixes: csi-addons#105

Signed-off-by: yati1998 <ypadia@redhat.com>
@mergify mergify bot merged commit e515dd5 into csi-addons:main Jan 31, 2022
nixpanic pushed a commit to nixpanic/kubernetes-csi-addons that referenced this pull request Jan 12, 2024
Syncing latest changes from upstream main for kubernetes-csi-addons
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate the manifest files for crds and controller manager
5 participants