-
Notifications
You must be signed in to change notification settings - Fork 0
Operator envs #1
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
Conversation
Reviewer's GuideThis PR introduces configurable timing parameters for the operator by parsing new environment variables and exposing them in deployment templates, refactors environment variable parsing logic for increased robustness, and unifies controller-runtime manager options setup using the common operator library with standardized imports. Sequence Diagram: Operator Manager Configuration with lib-commonsequenceDiagram
participant main as "main() (operator/main.go or main.go)"
participant libCommonOp as "lib-common/operator.SetManagerOptions"
participant ctrlOptions as "ctrl.Options"
participant ctrlMgr as "ctrl.NewManager"
main->>main: Initialize ctrl.Options (options)
note left of main: SetupEnv() has already parsed env vars like LEASE_DURATION into global variables
main->>libCommonOp: SetManagerOptions(&options, setupLog)
libCommonOp->>ctrlOptions: Configure leader election (LeaseDuration, RenewDeadline, RetryPeriod, etc.)
note right of libCommonOp: Uses values from env vars (read by SetupEnv()) either directly or indirectly if SetManagerOptions accesses them or they are passed through options struct setup before this call.
main->>ctrlMgr: NewManager(config, options)
Sequence Diagram: Refactored Environment Variable Parsing in SetupEnvsequenceDiagram
participant osEnviron as "os.Environ()"
participant SetupEnv as "SetupEnv()"
participant GlobalConfigs as "Global Config Variables"
SetupEnv->>osEnviron: Get all environment variables
loop For each environment variable
SetupEnv->>SetupEnv: Parse key & value (using strings.SplitN)
alt key is "LEASE_DURATION"
SetupEnv->>GlobalConfigs: operatorLeaseDuration = value
else key is "RENEW_DEADLINE"
SetupEnv->>GlobalConfigs: operatorRenewDeadline = value
else key is "RETRY_PERIOD"
SetupEnv->>GlobalConfigs: operatorRetryPeriod = value
else key matches "*_OPERATOR_MANAGER_IMAGE_URL"
SetupEnv->>GlobalConfigs: envRelatedOperatorImages[operatorName] = value
else key matches "RELATED_IMAGE_*"
SetupEnv->>GlobalConfigs: envRelatedOpenStackServiceImages[key] = value
else Other specific keys (KUBE_RBAC_PROXY, etc.)
SetupEnv->>GlobalConfigs: (assigns to respective global var)
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@sourcery-ai review |
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.
Hey @stuggi - I've reviewed your changes - here's some feedback:
- Provide in-code default values for LEASE_DURATION, RENEW_DEADLINE, and RETRY_PERIOD to mirror your template defaults and avoid running with empty settings when those env vars aren’t set.
- Simplify SetupEnv by using os.LookupEnv (or a map of expected env‐vars) instead of iterating over all os.Environ and splitting each entry, to make the parsing clearer and more efficient.
- Since you renamed the CRD field frrConfigurationNamespace to nodeName, consider adding a conversion webhook or migration logic to handle existing instances that still use the old field name.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Hey @stuggi - I've reviewed your changes - here's some feedback:
- In SetupEnv you’re taking the address of the reused loop variable
value, so every map entry will point to the same memory—capture a new local variable (e.g.val := value) per case before storing its pointer. - The CRD change renaming
frrConfigurationNamespacetonodeNameis a breaking change—consider bumping the CRD version or adding conversion logic to preserve compatibility.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
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.
Hey @stuggi - I've reviewed your changes - here's some feedback:
- In SetupEnv, use strings.SplitN(env, "=", 2) instead of strings.Split so you correctly capture values containing “=”.
- Add validation or fallback defaults for LEASE_DURATION, RENEW_DEADLINE, and RETRY_PERIOD to avoid starting the operator with empty or invalid timing values.
- The manager options initialization is duplicated in cmd/operator/main.go and main.go—consider extracting it into a shared helper to reduce duplication.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| - name: LEASE_DURATION | ||
| value: "137" | ||
| - name: RENEW_DEADLINE | ||
| value: "107" | ||
| - name: RETRY_PERIOD |
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.
suggestion: Hardcoded defaults may drift without context
Consider centralizing the default values (137, 107, 26) or adding a comment explaining their choice to avoid them drifting out of sync.
|
@sourcery-ai review |
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.
Hey @stuggi - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary by Sourcery
Introduce configurable leader election parameters for the operator by wiring new environment variables through the setup and deployment, and refactor application startup to leverage lib-common for manager options.
New Features:
Enhancements:
Deployment: