-
Notifications
You must be signed in to change notification settings - Fork 23
Plane-EE: Configurable busybox image #178
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
base: master
Are you sure you want to change the base?
Conversation
|
Warning Rate limit exceeded@hajowieland has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughHelm templates for MinIO and Silo workloads are updated to use configurable busybox images via templated values instead of hardcoded references. New configuration fields added to the values file. Minor formatting adjustments applied across templates. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
charts/plane-enterprise/values.yaml (1)
87-87: Pin specific BusyBox image versions instead of using untaggedbusybox.Both defaults (
busyboxwithout tag) will pull the latest image, violating reproducibility and introducing unnecessary variability. Other services in this file pin specific versions (e.g.,valkey:7.2.11-alpine,postgres:15.7-alpine). Pin BusyBox to a stable version such asbusybox:1.37.0-alpineto match the immutability principle across the chart.minio: local_setup: true image: minio/minio:latest image_mc: minio/mc:latest - image_busybox: busybox + image_busybox: busybox:1.37.0-alpine volumeSize: 3Gi silo: enabled: true replicas: 1 memoryLimit: 1000Mi cpuLimit: 500m memoryRequest: 50Mi cpuRequest: 50m image: artifacts.plane.so/makeplane/silo-commercial - busyboxImage: busybox + image_busybox: busybox:1.37.0-alpine pullPolicy: AlwaysAlso applies to: 239-239
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
charts/plane-enterprise/templates/workloads/minio.stateful.yaml(4 hunks)charts/plane-enterprise/templates/workloads/silo.deployment.yaml(2 hunks)charts/plane-enterprise/values.yaml(2 hunks)
🔇 Additional comments (4)
charts/plane-enterprise/templates/workloads/silo.deployment.yaml (2)
46-46: Verify template reference matches finalized values.yaml key name.This line references
.Values.services.silo.busyboxImage, but due to the naming inconsistency noted in values.yaml, this reference may need to change to.Values.services.silo.image_busyboxif the field is standardized to snake_case.Confirm that the template reference aligns with the corrected values.yaml field naming convention.
52-61: Unexpected logic change beyond scope: conditional RabbitMQ wait behavior.The AI summary describes "minor formatting adjustments," but lines 52–61 introduce significant behavioral changes: a conditional check that skips the RabbitMQ wait if an external RabbitMQ URL is detected. This goes beyond configuring the BusyBox image and represents a distinct feature addition.
Clarify whether this logic change is intentional (and if so, verify it's correct) or should be separated into a distinct PR.
- Is the conditional RabbitMQ detection intentional or scope creep?
- Should minio's init container (in the other template) have similar logic?
- Does this behavior align with the project's goals for this PR?
Provide context on why this change was bundled with the busybox configurability fix.
charts/plane-enterprise/templates/workloads/minio.stateful.yaml (2)
100-100: ✓ BusyBox image templating is correct.The reference to
.Values.services.minio.image_busyboxis consistent with values.yaml and replaces the hardcoded image as intended. However, verify that this field name aligns with the standardization correction suggested in the values.yaml review (if minio field is renamed, ensure this reference follows).
111-113: Command formatting is acceptable.The reformatting of the mc configuration and bucket creation commands (separating them onto distinct lines with proper semicolon handling) improves readability without changing behavior. The command semantics are preserved.
Description
The
busyboximage used by MinIO and Silo is hardcoded in the Helm templates and not configurable. This is problematic, as users might want to pin the version, use a custom image, or need to pull from their own registry.Type of Change
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.