-
Notifications
You must be signed in to change notification settings - Fork 0
fix: dev envs #138
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
fix: dev envs #138
Conversation
Warning Rate limit exceeded@gfyrag has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 13 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 (1)
WalkthroughThe deployment logic in the Earthfile was updated to always set the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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: 0
🧹 Nitpick comments (1)
Earthfile (1)
120-127
: Consider cleanup: Conditional logic still references FORMANCE_DEV_CLUSTER_V2The conditional block for
FORMANCE_DEV_CLUSTER_V2
remains but no longer affectsserver.address
. Since the server address is now standardized, consider whether this conditional logic is still necessary or if it should be simplified.If the conditional behavior is still needed for the other parameters (imagePullSecrets, TLS settings, monitoring endpoints), the current implementation is fine. However, if the goal is to fully standardize dev environment configuration, you might consider removing or simplifying this conditional logic as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Earthfile
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Dirty
🔇 Additional comments (1)
Earthfile (1)
135-135
: Standardizing server.address – please verify environment impactThe unconditional
--set server.address=membership.formance.svc.cluster.local:8082
aligns with the PR’s goal to simplify dev deployments. Before merging, please:• Confirm there are no remaining conditional references to
$user.$tld
or$user.$tld:443
in the Earthfile (e.g. around theFORMANCE_DEV_CLUSTER_V2
block on lines ~121–127).
• Validate that production and any non-dev environments still resolve to the intended host/port.
• Clean up any now-unused conditional logic or comments related to server.address.
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.
Bug: Server Address Regression in Non-Dev
The conditional logic for server.address
was removed, causing it to unconditionally be membership.formance.svc.cluster.local:8082
. Previously, non-dev environments (when FORMANCE_DEV_CLUSTER_V2
was not 'yes') used $user.$tld:443
. This change forces non-dev deployments to use the internal dev address, potentially breaking them. Additionally, other development-specific settings (e.g., TLS, imagePullSecrets) remain conditional, leading to an inconsistent configuration for non-dev environments.
Earthfile#L120-L136
Lines 120 to 136 in b7a6384
ARG FORMANCE_DEV_CLUSTER_V2=no | |
IF [ "$FORMANCE_DEV_CLUSTER_V2" == "yes" ] | |
SET ADDITIONAL_ARGS="$ADDITIONAL_ARGS --set imagePullSecrets[0].name=zot" | |
SET ADDITIONAL_ARGS="$ADDITIONAL_ARGS --set server.tls.enabled=false" | |
SET ADDITIONAL_ARGS="$ADDITIONAL_ARGS --set global.monitoring.traces.endpoint=otel-shared-admin.default.svc.cluster.local" | |
SET ADDITIONAL_ARGS="$ADDITIONAL_ARGS --set global.monitoring.metrics.endpoint=otel-shared-admin.default.svc.cluster.local" | |
SET ADDITIONAL_ARGS="$ADDITIONAL_ARGS --set image.repository=$REPOSITORY/formancehq/agent" | |
END | |
RUN --secret tld helm upgrade --namespace formance \ | |
--create-namespace \ | |
--install \ | |
--wait \ | |
-f .earthly/values.yaml \ | |
--set image.tag=$tag \ | |
--set agent.baseUrl=https://$user.$tld \ | |
--set server.address=membership.formance.svc.cluster.local:8082 \ | |
formance-membership-agent ./helm $ADDITIONAL_ARGS |
Was this report helpful? Give feedback by reacting with 👍 or 👎
No description provided.