refactor(helm): support crd upgrader#175
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Helm chart deployment process to provide more robust handling of Custom Resource Definitions (CRDs). It introduces a dedicated CRD Upgrader Job to manage CRD installation and upgrades, especially for large CRD files that might exceed Helm's secret size limits. The changes also include updated Makefile targets for better CRD lifecycle management and comprehensive documentation for the new deployment methods. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a CRD upgrader job for the Helm chart, which is a good strategy for managing large CRDs that can exceed Helm's storage limits. The changes are well-structured, including new Makefile targets, updates to Helm templates and values, and comprehensive documentation updates. My review includes suggestions to improve the robustness of the deployment scripts and enhance consistency within the Makefile.
|
|
||
| .PHONY: helm-undeploy | ||
| helm-undeploy: ## Undeploy controller installed via Helm from the K8s cluster. | ||
| $(HELM) uninstall rbgs --namespace rbgs-system --ignore-not-found || true |
There was a problem hiding this comment.
The --ignore-not-found flag is not valid for the helm uninstall command and will cause an error. The || true already handles the case where the release doesn't exist. Please remove the invalid flag.
$(HELM) uninstall rbgs --namespace rbgs-system || true
| docker-buildx-push-crd-upgrader: ## Build and push CRD Upgrader image for cross-platform support | ||
| - $(CONTAINER_TOOL) buildx create --name rbgs-crd-builder | ||
| $(CONTAINER_TOOL) buildx use rbgs-crd-builder | ||
| - $(CONTAINER_TOOL) buildx build --push --platform=linux/amd64,linux/arm64 --tag ${CRD_UPGRADER_IMG}:${TAG} -f ${CRD_UPGRADER_DOCKERFILE} . |
There was a problem hiding this comment.
The build platforms for the CRD upgrader image are hardcoded. For better consistency and maintainability, consider using the $(PLATFORMS) variable, which is already used for building the main controller image. This centralizes platform management.
- $(CONTAINER_TOOL) buildx build --push --platform=$(PLATFORMS) --tag ${CRD_UPGRADER_IMG}:${TAG} -f ${CRD_UPGRADER_DOCKERFILE} .
|
|
||
| # Copy CRD files and upgrade script | ||
| COPY ./deploy/helm/rbgs/crds /rbgs/crds | ||
| COPY ./tools/crd-upgrade/upgrade-crds.sh /rbgs/upgrade-crds.sh |
There was a problem hiding this comment.
The upgrade-crds.sh script can be significantly simplified and made more robust. Instead of looping and using kubectl create or kubectl replace, it's better to use kubectl apply. kubectl apply is idempotent and is the standard practice for managing Kubernetes resources declaratively, handling both initial creation and subsequent updates correctly.
I recommend changing the content of tools/crd-upgrade/upgrade-crds.sh to:
#!/bin/bash
set -e
echo "Applying CRDs from /rbgs/crds/..."
kubectl apply -f /rbgs/crds/
echo "CRDs applied successfully."This aligns with Kubernetes best practices and the patch permission that has been added to the associated ClusterRole.
Makefile
Outdated
| --create-namespace \ | ||
| --namespace rbgs-system \ | ||
| --set image.tag=$(TAG) \ | ||
| --skip-crds \ |
There was a problem hiding this comment.
I suggest removing the CRD files from the Helm chart so that users can install it without needing a Makefile.
2 participants
Ⅰ. Motivation
Ⅱ. Modifications
Ⅲ. Does this pull request fix one issue?
fixes #XXXX
Ⅳ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅴ. Describe how to verify it
VI. Special notes for reviews
Checklist
make fmt.