feat: devenv#292
Conversation
WalkthroughThis update refactors image handling throughout the codebase by introducing a unified Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant Registries
participant Settings
participant Kubernetes
Controller->>Registries: GetFormanceImage(ctx, stack, name, version)
Registries->>Settings: Query image/registry overrides
Registries->>Registries: Parse registry endpoint for pull secrets
Registries-->>Controller: Return ImageConfiguration (image, pull secrets)
Controller->>Kubernetes: Deploy resource (image=ImageConfiguration.GetFullImageName(), imagePullSecrets=ImageConfiguration.PullSecrets)
Kubernetes-->>Controller: Resource created with correct image and pull secrets
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected related to the linked issue objectives. Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (34)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (26)
🧰 Additional context used🧬 Code Graph Analysis (1)internal/resources/databases/migrate.go (6)
🔇 Additional comments (6)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Earthfile (1)
71-71: Consider providing a default value for the REPOSITORY argument.The
REPOSITORYargument is required but lacks a default value, forcing all callers to explicitly provide it. Consider adding a sensible default to improve usability.- ARG --required REPOSITORY= + ARG REPOSITORY=ghcr.io
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Earthfile(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Dirty
🔇 Additional comments (2)
Earthfile (2)
3-4: LGTM! Good parameterization of the core image.The parameterization of the core image import allows for flexible version management, which is a best practice for build configurations.
88-88: LGTM! Consistent namespace change.The namespace change from
formance-systemtoformanceis consistent with the operator deployment above.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
internal/resources/ledgers/deployments.go (3)
249-266: Critical: Missing ImagePullSecrets configuration in pod spec.The deployment template sets the container image but doesn't configure
ImagePullSecretson the pod spec, unlike the payments deployments. This will prevent authentication with private registries.Apply this diff to add the missing image pull secrets:
tpl := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ Name: "ledger", }, Spec: appsv1.DeploymentSpec{ Template: corev1.PodTemplateSpec{ Spec: corev1.PodSpec{ Containers: []corev1.Container{container}, ServiceAccountName: serviceAccountName, + ImagePullSecrets: imageConfiguration.PullSecrets, }, }, }, }
284-302: Critical: Missing ImagePullSecrets configuration in worker deployment.Similar to the stateless deployment, the worker deployment template doesn't configure
ImagePullSecrets.Apply this diff to add the missing image pull secrets:
tpl := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ Name: "ledger-worker", }, Spec: appsv1.DeploymentSpec{ Template: corev1.PodTemplateSpec{ Spec: corev1.PodSpec{ Containers: []corev1.Container{container}, ServiceAccountName: serviceAccountName, + ImagePullSecrets: imageConfiguration.PullSecrets, }, }, }, }
387-405: Critical: Missing ImagePullSecrets configuration in createDeployment function.The
createDeploymenthelper function creates deployment templates but doesn't configureImagePullSecrets, which affects all deployments that use this helper.Apply this diff to add the missing image pull secrets:
tpl := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ Name: name, }, Spec: appsv1.DeploymentSpec{ Template: corev1.PodTemplateSpec{ Spec: corev1.PodSpec{ Containers: []corev1.Container{container}, Volumes: volumes, ServiceAccountName: serviceAccountName, + ImagePullSecrets: imageConfiguration.PullSecrets, }, }, }, }However, this function doesn't have access to
imageConfiguration. The function signature needs to be updated to accept the image configuration parameter:-func createDeployment(ctx core.Context, stack *v1beta1.Stack, ledger *v1beta1.Ledger, name string, container corev1.Container, v2 bool, replicas uint64) error { +func createDeployment(ctx core.Context, stack *v1beta1.Stack, ledger *v1beta1.Ledger, name string, container corev1.Container, imageConfiguration *registries.ImageConfiguration, v2 bool, replicas uint64) error {And update all calls to this function to pass the imageConfiguration parameter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.earthly/configuration/templates/settings.yamlis excluded by!**/*.yaml
📒 Files selected for processing (30)
api/formance.com/v1beta1/benthos_types.go(1 hunks)docs/09-Configuration reference/01-Settings.md(2 hunks)internal/resources/auths/deployment.go(3 hunks)internal/resources/auths/init.go(3 hunks)internal/resources/benthos/controller.go(1 hunks)internal/resources/brokerconsumers/controller.go(4 hunks)internal/resources/brokers/reconcile.go(7 hunks)internal/resources/caddy/caddy.go(4 hunks)internal/resources/databases/init.go(2 hunks)internal/resources/databases/migrate.go(1 hunks)internal/resources/gateways/deployment.go(1 hunks)internal/resources/jobs/job.go(2 hunks)internal/resources/ledgers/deployments.go(10 hunks)internal/resources/ledgers/init.go(4 hunks)internal/resources/orchestrations/deployments.go(3 hunks)internal/resources/orchestrations/init.go(2 hunks)internal/resources/payments/deployments.go(10 hunks)internal/resources/payments/finalizer.go(2 hunks)internal/resources/payments/init.go(4 hunks)internal/resources/reconciliations/deployments.go(2 hunks)internal/resources/reconciliations/init.go(2 hunks)internal/resources/registries/image.go(1 hunks)internal/resources/registries/registries.go(2 hunks)internal/resources/registries/registries_generated.go(0 hunks)internal/resources/registries/registries_test.go(0 hunks)internal/resources/searches/init.go(3 hunks)internal/resources/stargates/deployment.go(2 hunks)internal/resources/wallets/deployment.go(2 hunks)internal/resources/webhooks/deployment.go(4 hunks)internal/resources/webhooks/init.go(1 hunks)
💤 Files with no reviewable changes (2)
- internal/resources/registries/registries_generated.go
- internal/resources/registries/registries_test.go
✅ Files skipped from review due to trivial changes (1)
- api/formance.com/v1beta1/benthos_types.go
🧰 Additional context used
🧬 Code Graph Analysis (16)
internal/resources/databases/init.go (2)
internal/resources/registries/image.go (1)
GetFormanceImage(24-30)internal/resources/jobs/job.go (3)
Mutator(37-41)WithServiceAccount(43-48)WithImagePullSecrets(100-105)
internal/resources/payments/init.go (4)
internal/resources/registries/image.go (1)
GetFormanceImage(24-30)internal/resources/databases/migrate.go (1)
Migrate(15-46)internal/resources/jobs/job.go (1)
WithEnvVars(50-55)internal/core/env.go (1)
Env(11-16)
internal/resources/payments/finalizer.go (2)
internal/resources/registries/image.go (1)
GetFormanceImage(24-30)internal/resources/jobs/job.go (1)
WithImagePullSecrets(100-105)
internal/resources/jobs/job.go (1)
internal/core/env.go (1)
Env(11-16)
internal/resources/stargates/deployment.go (2)
internal/resources/registries/image.go (1)
GetFormanceImage(24-30)internal/core/env.go (1)
Env(11-16)
internal/resources/brokerconsumers/controller.go (1)
internal/resources/jobs/job.go (1)
WithImagePullSecrets(100-105)
internal/resources/webhooks/init.go (4)
internal/resources/registries/image.go (1)
GetFormanceImage(24-30)internal/core/version.go (1)
IsGreaterOrEqual(44-52)internal/resources/databases/create.go (1)
GetSavedModuleVersion(69-71)internal/resources/databases/migrate.go (1)
Migrate(15-46)
internal/resources/gateways/deployment.go (2)
internal/resources/registries/image.go (1)
GetFormanceImage(24-30)internal/resources/caddy/caddy.go (1)
DeploymentTemplate(22-95)
internal/resources/brokers/reconcile.go (3)
internal/resources/jobs/job.go (1)
WithImagePullSecrets(100-105)internal/core/shell.go (1)
ShellScript(5-12)internal/core/env.go (1)
Env(11-16)
internal/resources/reconciliations/init.go (4)
internal/resources/registries/image.go (1)
GetFormanceImage(24-30)internal/core/version.go (1)
IsGreaterOrEqual(44-52)internal/resources/databases/create.go (1)
GetSavedModuleVersion(69-71)internal/resources/databases/migrate.go (1)
Migrate(15-46)
internal/resources/caddy/caddy.go (4)
internal/core/context.go (1)
Context(10-16)api/formance.com/v1beta1/stack_types.go (1)
Stack(77-83)api/formance.com/v1beta1/shared.go (1)
Object(294-300)internal/resources/registries/registries.go (1)
ImageConfiguration(18-23)
internal/resources/orchestrations/deployments.go (8)
internal/core/context.go (1)
Context(10-16)api/formance.com/v1beta1/stack_types.go (1)
Stack(77-83)api/formance.com/v1beta1/orchestration_types.go (1)
Orchestration(47-53)api/formance.com/v1beta1/database_types.go (1)
Database(73-79)api/formance.com/v1beta1/authclient_types.go (1)
AuthClient(90-96)api/formance.com/v1beta1/brokerconsumer_types.go (1)
BrokerConsumer(45-51)internal/resources/registries/registries.go (1)
ImageConfiguration(18-23)internal/core/env.go (1)
Env(11-16)
internal/resources/reconciliations/deployments.go (7)
internal/core/context.go (1)
Context(10-16)api/formance.com/v1beta1/stack_types.go (1)
Stack(77-83)api/formance.com/v1beta1/reconciliation_types.go (1)
Reconciliation(45-51)api/formance.com/v1beta1/database_types.go (1)
Database(73-79)api/formance.com/v1beta1/authclient_types.go (1)
AuthClient(90-96)internal/resources/registries/registries.go (1)
ImageConfiguration(18-23)internal/core/env.go (1)
Env(11-16)
internal/resources/registries/registries.go (2)
internal/core/context.go (1)
Context(10-16)internal/resources/settings/helpers.go (1)
GetStringOrEmpty(53-55)
internal/resources/registries/image.go (4)
internal/core/context.go (1)
Context(10-16)api/formance.com/v1beta1/stack_types.go (1)
Stack(77-83)internal/resources/registries/registries.go (2)
ImageConfiguration(18-23)GetImageConfiguration(39-94)internal/resources/settings/helpers.go (1)
GetStringOrDefault(42-51)
internal/resources/ledgers/deployments.go (7)
internal/core/context.go (1)
Context(10-16)api/formance.com/v1beta1/stack_types.go (1)
Stack(77-83)api/formance.com/v1beta1/ledger_types.go (2)
Ledger(97-103)DeploymentStrategyMonoWriterMultipleReader(52-52)api/formance.com/v1beta1/database_types.go (1)
Database(73-79)internal/resources/registries/registries.go (1)
ImageConfiguration(18-23)internal/core/env.go (1)
Env(11-16)internal/resources/registries/image.go (1)
GetCaddyImage(48-56)
🔇 Additional comments (89)
internal/resources/benthos/controller.go (2)
323-323: LGTM: Proper image pull secrets integration.The merge of
benthosImage.PullSecretswithb.Spec.ImagePullSecretscorrectly allows both registry-configured secrets and Benthos-specific additional secrets to be used.
327-327: LGTM: Correct usage of image configuration.Using
GetFullImageName()from the image configuration object provides the properly qualified image reference.internal/resources/gateways/deployment.go (2)
29-32: LGTM: Consistent image configuration usage.The switch from
registries.GetImagetoregistries.GetFormanceImagecorrectly returns anImageConfigurationobject instead of a plain string, enabling proper image pull secret handling.
34-34: LGTM: Proper integration with deployment template.Passing the
imageConfigurationobject tocaddy.DeploymentTemplatealigns with the updated function signature that expects anImageConfigurationparameter.docs/09-Configuration reference/01-Settings.md (1)
45-45: LGTM: Clear documentation of pull secret configuration.The example effectively demonstrates how to specify image pull secrets via the
pullSecretquery parameter in registry endpoint settings, which aligns with the code changes that parse and apply these secrets.internal/resources/databases/init.go (3)
141-144: LGTM: Consistent image configuration retrieval.Using
registries.GetFormanceImagefor the operator-utils image correctly returns anImageConfigurationobject with both image reference and pull secrets.
172-172: LGTM: Proper image reference usage.Using
GetFullImageName()from the image configuration ensures the container gets the fully qualified image name.
178-178: LGTM: Essential security improvement for database jobs.Adding
jobs.WithImagePullSecrets(operatorUtilsImage.PullSecrets)ensures database jobs can authenticate to private registries when pulling the operator-utils image, which is critical for proper job execution.internal/resources/stargates/deployment.go (3)
42-45: LGTM: Consistent image configuration pattern.The switch to
registries.GetFormanceImagecorrectly returns anImageConfigurationobject instead of a plain string, enabling proper handling of both image reference and pull secrets.
55-55: LGTM: Essential security enhancement.Adding
ImagePullSecrets: imageConfiguration.PullSecretsto the pod spec enables authentication to private registries, which is crucial for enterprise deployments.
59-59: LGTM: Proper image reference formatting.Using
GetFullImageName()from the image configuration ensures the container gets the correctly formatted, fully qualified image name.internal/resources/orchestrations/init.go (3)
61-64: LGTM: Image configuration retrieval updated correctly.The change from
registries.GetImagetoregistries.GetFormanceImagealigns with the unified image configuration refactor. The error handling remains consistent.
68-70: Simplified migration logic is an architectural improvement.The migration logic has been consolidated from explicit job creation to a single
databases.Migratecall that now handles the image configuration internally. This simplification reduces complexity and improves maintainability.
78-80: Deployment creation correctly updated for image configuration.The
createDeploymentfunction call now receives theImageConfigurationobject instead of a plain image string, maintaining consistency with the refactor pattern.internal/resources/wallets/deployment.go (3)
43-46: Image configuration retrieval correctly implemented.The switch to
registries.GetFormanceImagefollows the established refactor pattern and properly handles theImageConfigurationreturn type.
56-56: Image pull secrets properly added for private registry support.Adding
ImagePullSecretsfrom the image configuration enhances security by enabling authentication with private container registries.
61-61: Container image reference correctly updated.Using
imageConfiguration.GetFullImageName()provides the fully qualified image name while maintaining consistency with the new image configuration approach.internal/resources/searches/init.go (3)
112-115: Image configuration retrieval correctly implemented.The change to
GetFormanceImagealigns with the unified image configuration refactor and maintains proper error handling.
153-157: Benthos init container and image pull secrets properly configured.Both the init container image and the image pull secrets are correctly set using the
ImageConfigurationobject, ensuring consistent image handling for the Benthos resource.
183-186: Deployment image configuration correctly applied.The deployment pod spec properly includes both the image pull secrets and the fully qualified image name, maintaining consistency with the refactor pattern.
internal/resources/payments/finalizer.go (2)
32-35: Image configuration retrieval correctly implemented.The change to
registries.GetFormanceImagefollows the established refactor pattern and maintains proper error handling.
48-49: Job container and image pull secrets properly configured.Both the container image and the image pull secrets are correctly configured using the
ImageConfigurationobject, ensuring the cleanup job can access private registries if needed.internal/resources/jobs/job.go (1)
100-105: Image pull secrets function correctly implemented.The
WithImagePullSecretsfunction safely appends to the existing slice and follows the establishedHandleJobOptionpattern. This enables Jobs to authenticate with private container registries.internal/resources/reconciliations/init.go (1)
49-65: LGTM! Consistent image configuration refactor.The changes properly implement the new image configuration pattern:
- Correctly uses
GetFormanceImageto retrieveImageConfigurationwith pull secrets- Simplifies migration logic by delegating to
databases.Migrate- Maintains proper error handling throughout
This refactor improves support for private registries while reducing code duplication.
internal/resources/webhooks/init.go (1)
54-68: LGTM! Consistent refactor implementation.The changes align perfectly with the image configuration standardization:
- Proper use of
GetFormanceImagefor structured image handling- Simplified migration via
databases.Migrateremoves code duplication- Clear variable naming with
imageConfigurationinternal/resources/brokers/reconcile.go (4)
123-158: LGTM! Proper image configuration usage in job handling.The changes correctly implement the new image pattern:
- Uses
GetFullImageName()for explicit image references- Includes
WithImagePullSecrets()for private registry authentication- Maintains existing job logic and error handling
201-210: LGTM! Consistent pattern applied to delete operations.The broker deletion logic properly uses the image configuration pattern while maintaining the original shell script logic.
238-253: LGTM! Stream creation follows the established pattern.The
createOneStreamByStackfunction correctly implements the image configuration usage with proper pull secret handling.
299-315: LGTM! Topic creation maintains consistency.The
createNatsTopicfunction properly applies the image configuration pattern while preserving the NATS-specific logic.internal/resources/brokerconsumers/controller.go (2)
190-201: LGTM! Service consumer creation follows image configuration pattern.The function correctly implements:
- Full image name usage via
GetFullImageName()- Image pull secrets for private registry access
- Maintains original NATS consumer logic
248-264: LGTM! Stack consumer creation maintains consistency.The
createStackNatsConsumerfunction properly applies the image configuration pattern while preserving the complex subject filtering logic.internal/resources/payments/init.go (2)
55-75: LGTM! Proper image configuration and migration updates.The changes correctly implement the refactor pattern:
- Uses
GetFormanceImagefor structured image handling- Properly passes encryption key via
jobs.WithEnvVars()to migration- Maintains existing error handling and version logic
80-103: LGTM! Deployment creation calls updated consistently.All deployment creation functions (
createFullDeployment,createReadDeployment,createConnectorsDeployment) are properly updated to use the newimageConfigurationparameter while maintaining version-specific logic.internal/resources/reconciliations/deployments.go (4)
11-11: Import addition aligns with the refactoring.The import for the registries package is correctly added to support the new ImageConfiguration parameter type.
18-25: Function signature change follows the refactoring pattern.The function signature has been updated from accepting a string
imageparameter to accepting an*registries.ImageConfigurationparameter. This change is consistent with the broader refactoring described in the AI summary and enables unified image configuration management including pull secrets support.
68-68: Image pull secrets correctly added to pod spec.The pod spec now includes
ImagePullSecretsfrom the image configuration, which is essential for supporting private container registries. This follows Kubernetes best practices for authenticated image pulls.
73-73: Container image correctly uses the full image name.The container image is now set using
imageConfiguration.GetFullImageName(), which provides the complete image reference including registry, name, and version. This is the correct way to utilize the new ImageConfiguration structure.internal/resources/webhooks/deployment.go (7)
74-74: Correctly updated to use the new image configuration function.The call has been changed from
registries.GetImagetoregistries.GetFormanceImage, which now returns an*ImageConfigurationinstead of a plain string. This aligns with the refactoring to unify image configuration management.
114-114: Image pull secrets properly added to API deployment.The pod spec now includes
ImagePullSecretsfrom the image configuration, enabling support for private container registries in the API deployment.
118-118: Container image correctly uses full image name in API deployment.The container image reference has been updated to use
imageConfiguration.GetFullImageName(), which provides the complete image reference including registry, name, and version.
131-138: Function signature formatting improved for readability.The function parameters have been reformatted to be on separate lines, which improves readability without changing functionality. This is a good practice for functions with multiple parameters.
140-140: Correctly updated to use the new image configuration function in worker deployment.Consistent with the API deployment, the worker deployment now uses
registries.GetFormanceImageto get the image configuration.
168-168: Image pull secrets properly added to worker deployment.The worker deployment pod spec now includes
ImagePullSecretsfrom the image configuration, maintaining consistency with the API deployment.
173-173: Container image correctly uses full image name in worker deployment.The worker container image reference has been updated to use
imageConfiguration.GetFullImageName(), consistent with the API deployment.internal/resources/auths/deployment.go (4)
8-8: Import correctly added for the registries package.The import for the registries package is properly added to support the new ImageConfiguration parameter type.
33-34: Function signature updated to use ImageConfiguration.The function signature has been changed from accepting a string
imageparameter to accepting an*registries.ImageConfigurationparameter. This change is consistent with the broader refactoring and enables unified image configuration management.
154-154: Image pull secrets correctly added to pod spec.The pod spec now includes
ImagePullSecretsfrom the image configuration, which is essential for supporting private container registries.
159-159: Container image correctly uses the full image name.The container image is now set using
imageConfiguration.GetFullImageName(), which provides the complete image reference including registry, name, and version.internal/resources/caddy/caddy.go (4)
6-6: Import correctly added for the registries package.The import for the registries package is properly added to support the new ImageConfiguration parameter type.
22-29: Function signature updated to use ImageConfiguration.The
DeploymentTemplatefunction signature has been changed from accepting a stringimageparameter to accepting an*registries.ImageConfigurationparameter. This change is consistent with the broader refactoring and enables unified image configuration management.
63-63: Image pull secrets correctly added to pod spec.The pod template spec now includes
ImagePullSecretsfrom the image configuration, which is essential for supporting private container registries.
73-73: Container image correctly uses the full image name.The container image is now set using
imageConfiguration.GetFullImageName(), which provides the complete image reference including registry, name, and version.internal/resources/orchestrations/deployments.go (4)
5-5: Import correctly added for the registries package.The import for the registries package is properly added to support the new ImageConfiguration parameter type.
50-58: Function signature updated to use ImageConfiguration.The
createDeploymentfunction signature has been changed from accepting a stringimageparameter to accepting an*registries.ImageConfigurationparameter. This change is consistent with the broader refactoring and enables unified image configuration management.
187-187: Image pull secrets correctly added to pod spec.The pod spec now includes
ImagePullSecretsfrom the image configuration, which is essential for supporting private container registries.
192-192: Container image correctly uses the full image name.The container image is now set using
imageConfiguration.GetFullImageName(), which provides the complete image reference including registry, name, and version.internal/resources/auths/init.go (1)
81-99: LGTM! Good refactoring of image retrieval and migration logic.The changes properly implement the new
ImageConfigurationpattern and simplify the migration process by using the centralizeddatabases.Migratefunction.internal/resources/ledgers/init.go (2)
52-55: LGTM! Proper implementation of the new image configuration pattern.The change correctly uses
GetFormanceImageto retrieve theImageConfigurationobject.
99-129: Well-implemented migration logic with proper safety checks.The migration implementation correctly:
- Uses version-specific command modifications for backwards compatibility
- Safely deletes only deployments owned by this ledger instance before migration
- Properly passes environment variables to the migration container
The controller UID check ensures that only resources created by this specific ledger are affected.
internal/resources/databases/migrate.go (1)
15-46: Excellent refactoring of the migration function.The simplified
Migratefunction properly:
- Accepts the new
ImageConfigurationtype- Uses
GetFullImageName()for the container image- Includes image pull secrets for private registry support
- Maintains proper error handling and service account configuration
This is a clean implementation that aligns with the broader refactoring goals.
internal/resources/registries/image.go (1)
24-56: LGTM! Consistent implementation of the new image configuration pattern.All image retrieval functions have been properly updated to:
- Return
*ImageConfigurationinstead of raw strings- Use the centralized
GetImageConfigurationfunction- Maintain proper error handling
The
GetCaddyImagefunction correctly implements settings-based override with a sensible default fallback.internal/resources/registries/registries.go (2)
18-23: Good addition of PullSecrets to ImageConfiguration.The
ImageConfigurationstruct properly encapsulates all necessary image-related information including registry authentication via pull secrets.
39-94: Well-implemented image configuration parsing with pull secret support.The
GetImageConfigurationfunction provides:
- Proper validation of image format
- Support for image rewrite overrides via settings
- Elegant registry endpoint override with query parameter support
- Clear error messages for invalid configurations
The pull secret implementation via query parameters (e.g.,
?pullSecret=my-secret) is a clean approach for specifying registry authentication.internal/resources/payments/deployments.go (10)
190-197: LGTM: Function signature updated for unified image configuration.The function signature change from a plain image string to
*registries.ImageConfigurationaligns with the broader refactor to standardize image handling across the codebase.
255-255: LGTM: Function call updated to match new signature.The call to
createWorkerDeploymentcorrectly passes theimageConfigurationparameter.
270-270: LGTM: Proper image configuration and pull secrets implementation.The deployment correctly uses both the image pull secrets and the full image name from the configuration, enabling private registry authentication.
Also applies to: 275-275
293-293: LGTM: Function signature updated consistently.The worker deployment function signature is updated to match the unified image configuration pattern.
308-308: LGTM: Consistent image configuration usage.The worker deployment correctly implements both image pull secrets and full image name usage, maintaining consistency with other deployments.
Also applies to: 313-313
331-331: LGTM: Function signature updated for read deployment.The read deployment function signature follows the same pattern as other deployment functions.
358-358: LGTM: Image configuration properly implemented.The read deployment correctly uses both image pull secrets and the full image name.
Also applies to: 363-363
386-386: LGTM: Connectors deployment function signature updated.The function signature is consistent with the other deployment functions.
432-432: LGTM: Connectors deployment implements image configuration correctly.The connectors deployment follows the same pattern as other deployments for image configuration and pull secrets.
Also applies to: 437-437
475-475: LGTM: Simplified Caddy image retrieval.The call to
registries.GetCaddyImagehas been simplified by removing the explicit version parameter, which suggests the function now handles default version resolution internally.internal/resources/ledgers/deployments.go (16)
75-75: LGTM: Function signature updated for unified image configuration.The main installation function signature is updated to use
*registries.ImageConfigurationconsistently with the broader refactor.
81-81: LGTM: Function calls updated to pass image configuration.The calls to
installLedgerStatelessandinstallLedgerWorkercorrectly pass theimageConfigurationparameter.Also applies to: 85-85
107-107: LGTM: Function calls updated consistently.The calls to deployment strategy functions properly pass the
imageConfigurationparameter.Also applies to: 109-109
115-122: LGTM: Function signature updated for single instance deployment.The function signature follows the unified image configuration pattern.
128-128: LGTM: Function call updated to pass image configuration.The call to
setCommonAPIContainerConfigurationcorrectly passes theimageConfigurationparameter.
157-157: LGTM: Function signature updated for stateless deployment.The function signature is consistent with other deployment functions.
239-239: LGTM: Function call updated in stateless deployment.The call to
setCommonAPIContainerConfigurationproperly passes the image configuration.
268-268: LGTM: Worker function signature updated.The function signature follows the unified image configuration pattern.
274-274: LGTM: Function call updated in worker deployment.The call to
setCommonContainerConfigurationcorrectly passes the image configuration.
304-304: LGTM: Function signature updated for mono-writer deployment.The function signature is consistent with the refactor pattern.
307-307: LGTM: Function call updated in mono-writer deployment.The call to
setCommonAPIContainerConfigurationproperly passes the image configuration.
408-408: LGTM: Function signature updated for common container configuration.The function signature is updated to accept the image configuration parameter.
428-428: LGTM: Container image set using configuration.The container image is correctly set using
imageConfiguration.GetFullImageName().
436-436: LGTM: Function signature updated for API container configuration.The function signature follows the unified pattern.
443-443: LGTM: Function call updated in API container configuration.The call to
setCommonContainerConfigurationcorrectly passes the image configuration.
560-560: LGTM: Simplified Caddy image retrieval.Consistent with the payments file, the call to
registries.GetCaddyImagehas been simplified by removing the explicit version parameter.
| func WithEnvVars(envVars ...v1.EnvVar) HandleJobOption { | ||
| return Mutator(func(t *batchv1.Job) error { | ||
| t.Spec.Template.Spec.Containers[0].Env = append(t.Spec.Template.Spec.Containers[0].Env, envVars...) | ||
| return nil | ||
| }) | ||
| } |
There was a problem hiding this comment.
Potential array bounds violation in WithEnvVars.
The function assumes t.Spec.Template.Spec.Containers[0] exists, which could panic if called on a Job with no containers. Consider adding a bounds check.
func WithEnvVars(envVars ...v1.EnvVar) HandleJobOption {
return Mutator(func(t *batchv1.Job) error {
+ if len(t.Spec.Template.Spec.Containers) == 0 {
+ return errors.New("job has no containers")
+ }
t.Spec.Template.Spec.Containers[0].Env = append(t.Spec.Template.Spec.Containers[0].Env, envVars...)
return nil
})
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/resources/jobs/job.go around lines 50 to 55, the code assumes that
the Containers slice has at least one element, which can cause a panic if it is
empty. Add a check to verify that t.Spec.Template.Spec.Containers has at least
one container before accessing index 0. If no containers exist, return an error
or handle the case appropriately to avoid out-of-bounds access.
| }, | ||
| Spec: corev1.PodSpec{ | ||
| InitContainers: b.Spec.InitContainers, | ||
| ImagePullSecrets: append(benthosImage.PullSecrets, b.Spec.ImagePullSecrets...), |
There was a problem hiding this comment.
What happen if both are fullfilled ?
There was a problem hiding this comment.
From what I have understood of the doc, each secret will be tested for each image
| ImagePullSecrets: append(benthosImage.PullSecrets, b.Spec.ImagePullSecrets...), | ||
| InitContainers: b.Spec.InitContainers, |
There was a problem hiding this comment.
Can we use the same strategy as other container ?
There was a problem hiding this comment.
Sorry, I have not understood that point
c0602bd to
f274a86
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/resources/databases/migrate.go (2)
23-23: Consider making migration command configurable.The hardcoded
"migrate"command is appropriate for this specific function, but consider if different migration commands might be needed in the future.If flexibility is needed, consider:
- args := []string{"migrate"} + args := []string{"migrate"} // or accept as parameter if other commands needed
25-28: Inconsistent error handling pattern.The error handling here returns the error directly, while lines 32-33 wrap the error with context. Consider maintaining consistency throughout the function.
For consistency, consider wrapping this error with context:
env, err := GetPostgresEnvVars(ctx, stack, database) if err != nil { - return err + return errors.Wrap(err, "getting postgres environment variables") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.earthly/configuration/templates/settings.yamlis excluded by!**/*.yaml
📒 Files selected for processing (31)
Earthfile(2 hunks)api/formance.com/v1beta1/benthos_types.go(1 hunks)docs/09-Configuration reference/01-Settings.md(2 hunks)internal/resources/auths/deployment.go(3 hunks)internal/resources/auths/init.go(3 hunks)internal/resources/benthos/controller.go(1 hunks)internal/resources/brokerconsumers/controller.go(4 hunks)internal/resources/brokers/reconcile.go(7 hunks)internal/resources/caddy/caddy.go(4 hunks)internal/resources/databases/init.go(2 hunks)internal/resources/databases/migrate.go(1 hunks)internal/resources/gateways/deployment.go(1 hunks)internal/resources/jobs/job.go(2 hunks)internal/resources/ledgers/deployments.go(10 hunks)internal/resources/ledgers/init.go(4 hunks)internal/resources/orchestrations/deployments.go(3 hunks)internal/resources/orchestrations/init.go(2 hunks)internal/resources/payments/deployments.go(10 hunks)internal/resources/payments/finalizer.go(2 hunks)internal/resources/payments/init.go(4 hunks)internal/resources/reconciliations/deployments.go(2 hunks)internal/resources/reconciliations/init.go(2 hunks)internal/resources/registries/image.go(1 hunks)internal/resources/registries/registries.go(2 hunks)internal/resources/registries/registries_generated.go(0 hunks)internal/resources/registries/registries_test.go(0 hunks)internal/resources/searches/init.go(3 hunks)internal/resources/stargates/deployment.go(2 hunks)internal/resources/wallets/deployment.go(2 hunks)internal/resources/webhooks/deployment.go(4 hunks)internal/resources/webhooks/init.go(1 hunks)
💤 Files with no reviewable changes (2)
- internal/resources/registries/registries_generated.go
- internal/resources/registries/registries_test.go
🚧 Files skipped from review as they are similar to previous changes (28)
- docs/09-Configuration reference/01-Settings.md
- api/formance.com/v1beta1/benthos_types.go
- internal/resources/gateways/deployment.go
- internal/resources/orchestrations/init.go
- internal/resources/wallets/deployment.go
- internal/resources/webhooks/init.go
- internal/resources/benthos/controller.go
- internal/resources/searches/init.go
- internal/resources/payments/finalizer.go
- internal/resources/stargates/deployment.go
- internal/resources/databases/init.go
- internal/resources/reconciliations/init.go
- internal/resources/jobs/job.go
- internal/resources/auths/deployment.go
- internal/resources/orchestrations/deployments.go
- internal/resources/reconciliations/deployments.go
- internal/resources/payments/init.go
- internal/resources/brokers/reconcile.go
- internal/resources/brokerconsumers/controller.go
- internal/resources/auths/init.go
- internal/resources/ledgers/init.go
- internal/resources/webhooks/deployment.go
- internal/resources/caddy/caddy.go
- Earthfile
- internal/resources/registries/image.go
- internal/resources/registries/registries.go
- internal/resources/payments/deployments.go
- internal/resources/ledgers/deployments.go
🔇 Additional comments (4)
internal/resources/databases/migrate.go (4)
5-7: LGTM! Clean import additions for the refactored functionality.The new imports align well with the consolidated migration approach and image configuration standardization.
15-22: Well-designed function signature for consolidated migration.The function signature effectively consolidates migration functionality with good parameter organization and flexibility through variadic options.
30-33: Good error handling with contextual information.The error wrapping provides helpful context for debugging migration issues.
35-46: Effective job execution with proper configuration.The job handling correctly uses the new
ImageConfigurationapproach and properly passes through options including image pull secrets and service account configuration.
| ctx core.Context, | ||
| stack *v1beta1.Stack, | ||
| owner v1beta1.Dependent, | ||
| imageConfiguration *registries.ImageConfiguration, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding nil check for imageConfiguration parameter.
The function doesn't validate that imageConfiguration is non-nil, which could lead to a panic when calling GetFullImageName().
Add a nil check at the beginning of the function:
func Migrate(
ctx core.Context,
stack *v1beta1.Stack,
owner v1beta1.Dependent,
imageConfiguration *registries.ImageConfiguration,
database *v1beta1.Database,
options ...jobs.HandleJobOption,
) error {
+ if imageConfiguration == nil {
+ return errors.New("imageConfiguration cannot be nil")
+ }
args := []string{"migrate"}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| imageConfiguration *registries.ImageConfiguration, | |
| func Migrate( | |
| ctx core.Context, | |
| stack *v1beta1.Stack, | |
| owner v1beta1.Dependent, | |
| imageConfiguration *registries.ImageConfiguration, | |
| database *v1beta1.Database, | |
| options ...jobs.HandleJobOption, | |
| ) error { | |
| if imageConfiguration == nil { | |
| return errors.New("imageConfiguration cannot be nil") | |
| } | |
| args := []string{"migrate"} | |
| // ... rest of function ... | |
| } |
🤖 Prompt for AI Agents
In internal/resources/databases/migrate.go at line 19, add a nil check for the
imageConfiguration parameter at the start of the function to prevent potential
panics when calling GetFullImageName(). If imageConfiguration is nil, handle the
case appropriately, such as returning an error or logging a message, before
proceeding.
🚨 BugBot couldn't runBugBot is experiencing high demand right now. Try again in a few minutes by commenting "bugbot run" (requestId: serverGenReqId_eceb7d77-1b89-4341-8da9-7ca57aa5ecd6). |
14ad905 to
aad0b53
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Bug: Debug Code Leak in Production Logs
Accidentally committed debug code, including an import of github.com/davecgh/go-spew/spew and a call to spew.Dump(imageConfiguration). This outputs debug information to production logs and should be removed.
internal/resources/auths/init.go#L19-L85
operator/internal/resources/auths/init.go
Lines 19 to 85 in c0b0b31
Was this report helpful? Give feedback by reacting with 👍 or 👎
3 participants
Fixes OPE-2