Fix deduped volume mounts and env vars#66
Conversation
github-actions
bot
added
fork-pr
👋 Thanks, @galt-tr!This pull request comes from a fork. For security, our CI runs in a restricted mode.
Thanks for contributing to bsv-blockchain/teranode-operator! 🚀 |
There was a problem hiding this comment.
Pull request overview
This pull request adds deduplication logic for Kubernetes deployment configuration fields to prevent duplicate entries when applying overrides from custom resources. The changes include new utility functions for deduplicating environment variables, environment sources, image pull secrets, volumes, and volume mounts, along with comprehensive unit tests. The PR also updates dependencies to include testify for better test assertions and adjusts the replica status monitoring requeue interval in the PropagationReconciler.
Changes:
- Added five deduplication utility functions that keep the last occurrence of duplicate entries based on unique identifiers (env var names, configmap/secret references, volume names, etc.)
- Updated
SetDeploymentOverridesWithContextandSetClusterOverridesto apply deduplication when merging user-provided overrides with default deployment configurations - Increased the PropagationReconciler's replica status monitoring requeue interval from 1 second to 5 seconds
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| internal/utils/deployment.go | Added five deduplication functions and integrated them into the deployment override logic to prevent duplicate entries in environment variables, volumes, and secrets |
| internal/utils/deployment_test.go | Added comprehensive unit tests for all deduplication functions covering various scenarios including duplicates, empty slices, and order preservation |
| internal/controller/propagation_controller.go | Changed replica status monitoring requeue interval from 1 second to 5 seconds to reduce reconciliation frequency |
| go.mod | Added stretchr/testify dependency for improved test assertions and gopkg.in/yaml.v3 as indirect dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Keep only the last occurrence | ||
| for i, ef := range envFrom { | ||
| if key := getEnvFromKey(ef); key != "" && seen[key] == i { |
There was a problem hiding this comment.
The deduplicateEnvFrom function silently drops EnvFromSource entries that have neither ConfigMapRef nor SecretRef (when getEnvFromKey returns empty string). This could lead to unexpected behavior if such entries exist. Consider either documenting this behavior in the function comment, or handling entries with empty keys by keeping them without deduplication.
| // Keep only the last occurrence | |
| for i, ef := range envFrom { | |
| if key := getEnvFromKey(ef); key != "" && seen[key] == i { | |
| // Keep only the last occurrence; keep entries without a key as-is | |
| for i, ef := range envFrom { | |
| if key := getEnvFromKey(ef); key == "" || seen[key] == i { |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
2 participants
This pull request introduces deduplication logic for several Kubernetes deployment configuration fields to prevent duplicate entries when applying overrides, and adds corresponding unit tests to ensure correctness. It also updates dependencies and slightly adjusts the reconciliation requeue timing.
Deduplication improvements:
internal/utils/deployment.goto deduplicate environment variables, environment sources, image pull secrets, volumes, and volume mounts, always keeping the last occurrence of each unique item.SetDeploymentOverridesWithContextandSetClusterOverridesto use these deduplication functions when merging user overrides, ensuring no duplicate entries are introduced in the deployment spec. [1] [2] [3]Testing:
internal/utils/deployment_test.goto verify deduplication behavior for all new utility functions.Dependency updates:
github.com/stretchr/testifyfor improved testing assertions andgopkg.in/yaml.v3as an indirect dependency. [1] [2]Controller behavior:
PropagationReconcilerfrom 1 second to 5 seconds to reduce unnecessary reconciliation frequency.