feat: refine pulumi - #721
Conversation
WalkthroughThis pull request refactors the deployment logic and component management across multiple packages. The main function now integrates configuration loading and error handling directly into Pulumi’s runtime. New files introduce a comprehensive configuration system, modular API, storage, devbox, and worker components along with utility functions and CLI tools for JSON schema generation. Several legacy files have been removed and package names updated. Additionally, SQL triggers for transaction metadata and documentation updates (including a new schema reference) have been added. Changes
Sequence Diagram(s)sequenceDiagram
participant M as Main Function
participant C as Config.Load()
participant A as Component Constructors
participant R as Resource Registration
M->>C: Load configuration variables
C-->>M: Return configuration object
M->>A: Initialize API, Worker, Storage, Devbox components using config
A-->>M: Return component instances
M->>R: Register components and export deployment outputs
R-->>M: Resources registered and outputs available
sequenceDiagram
participant U as User
participant T as CLI Tool
participant S as printSchema Function
participant JS as JSON Schema Generator
U->>T: Execute "schema" command
T->>S: Invoke printSchema
S->>JS: Reflect on Config and generate JSON schema
JS-->>S: Return JSON schema
S-->>T: Output JSON schema to stdout
T-->>U: Display the schema documentation
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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:
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 (
|
228e7fd to
44ba8c5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #721 +/- ##
=======================================
Coverage 81.97% 81.97%
=======================================
Files 135 135
Lines 7291 7291
=======================================
Hits 5977 5977
Misses 1008 1008
Partials 306 306 ☔ View full report in Codecov by Sentry. |
44ba8c5 to
82e90ba
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (23)
deployments/pulumi/pkg/storage/migrate.go (1)
17-23: Be mindful of sensitive data in environment variables.
Appendingargs.component.GetEnvVars()might include sensitive keys. If any production secrets are needed, consider using PulumiSecretor other secure configurations to avoid leaking secrets in plain text.deployments/pulumi/pkg/utils/sync.go (1)
5-9: Clarify behavior when input is nil.Returning a zero value if
visnilmay lead to unexpected outputs in places where a valid value is required. Consider whether you should fail fast, log a warning, or otherwise indicate that no valid input was provided.deployments/pulumi/pkg/utils/convert.go (2)
8-10: Handle empty or nil tag gracefully.If
tagis empty or nil, the resulting image reference could be invalid. Consider defaulting to a safe tag or logging an error to avoid misconfigurations.
12-19: Rename function to better reflect return type.“BoolToString” is accurate, but adding “Output” in the name, such as “BoolToStringOutput,” may make it clearer that the function returns a
pulumix.Output[string].deployments/pulumi/pkg/api/service.go (1)
16-34: Check for namespace and deployment mismatches.Using
args.Namespace.ToOutput(ctx.Context()).Untyped().(pulumi.StringOutput)and directly referencingargs.Deployment.Spec.Selector()assume valid, consistent objects. If either is misconfigured, service creation may fail. Consider adding guard clauses or fallback logic.deployments/pulumi/pkg/devbox/component.go (1)
14-17: Add docstrings for struct fields.To promote maintainability, add descriptive comments to clarify the purpose of
ResourceStateandDeploymentin this component struct. This also helps future contributors.deployments/pulumi/main_test.go (1)
16-16: Consider potential type-safety tradeoffs usingmap[string]any.
While switching tomap[string]anyenables more complex configurations, be mindful of potential runtime errors if values are not type-checked or validated before use.deployments/pulumi/main.go (1)
12-15: Handle config loading errors more verbosely if needed.
Returningerron load failure is correct, though adding a descriptive message or limited logging could help diagnose configuration problems more quickly.deployments/pulumi/pkg/api/component.go (1)
19-24:ComponentArgsstruct introduced for clarity.
Using separate fields for common arguments, specialized args, storage, and ingress fosters good modularity. Consider adding doc comments for each field if more clarity is needed.deployments/pulumi/pkg/storage/setup_postgres.go (2)
17-32: Consider validating DSN fields in addition to the port.
Currently, only the port is guarded by error checks, but other fields like host, username, and password could also be invalid. Adding checks or returning a more descriptive error for invalid DSN fields would improve robustness.
44-44: Consider adding default logic or remove the empty function.
TheSetDefaultsmethod is currently empty. If you plan to set fallback values or environment-specific defaults, this is the right place to do so. Otherwise, removing it might reduce confusion.deployments/pulumi/pkg/component.go (2)
16-23: Double-check negative or zero timeouts.
Timeout pulumix.Input[int]inComponentArgscould be set to invalid or negative values. Consider setting a fallback or default if the input is invalid.
25-30: Ensure defaults are clearly distinguished from user-supplied values.
SetDefaultsmerges user-defined values with default settings. Log or document which fields, if any, are overridden to prevent confusion in different environments.deployments/pulumi/pkg/worker/component.go (2)
14-16: Use a more descriptive struct name or add doc comments.
Currently namedArgs, which matches other usage in the code. If there's no confusion with otherArgstypes, it's fine; otherwise, consider a more explicit name or doc comments clarifying its purpose.
44-48: Consolidate environment variable handling and nil checks.
Your code appends environment variables fromargs.Database.GetEnvVars()andargs.GetOpenTelemetryEnvVars(). Ensure these references are valid if corresponding fields arenil. A unified approach to environment variable assembly might reduce duplication and potential nil-pointer issues.Also applies to: 50-52
deployments/pulumi/pkg/storage/component_external.go (1)
47-52: Resource naming may be misleading for non-Postgres.
You named the resource"Formance:Ledger:ExternalPostgres"in the call toRegisterComponentResource. If you plan to support other DB engines, consider a more general resource string, e.g."Formance:Ledger:ExternalDatabase".Also applies to: 60-62
deployments/pulumi/pkg/utils/args.go (2)
43-49: Ensure defaults forImagePullPolicyalign with best practices.
Right now,ImagePullPolicydefaults to an empty string. Depending on your environment, consider a semantic default like"IfNotPresent"for consistent pull behavior.
73-213: Thorough environment variable assembly for OpenTelemetry.
The layering of environment variables is comprehensive. Ensure no secrets are inadvertently stored in logs or environment variables, especially in production.deployments/pulumi/pkg/api/deployment.go (1)
75-82: Repeated pattern for handling integer environment variables.You are repeating a pattern of using
pulumix.Apply(...)to conditionally transform zero values into empty strings. Consider a helper function that streamlines these transformations and reduces repetition.- corev1.EnvVarArgs{ - Name: pulumi.String("BULK_MAX_SIZE"), - Value: pulumix.Apply(args.BulkMaxSize, func(size int) string { - if size == 0 { - return "" - } - return fmt.Sprint(size) - }).Untyped().(pulumi.StringOutput), - }, + envVarInt(ctx, "BULK_MAX_SIZE", args.BulkMaxSize),deployments/pulumi/pkg/storage/component_postgres.go (1)
31-38: Hard-coded endpoint overrides dynamic resource resolution.
Currently,GetEndpoint()returns a fixed DNS (“postgres-postgresql.test-postgres.svc.cluster.local”), while the commented-out code shows a more dynamic approach using the Helm release’s actual name and namespace. Hard-coding can break multi-environment setups.func (cmp *PostgresDatabaseComponent) GetEndpoint() pulumix.Input[string] { - return pulumix.Val("postgres-postgresql.test-postgres.svc.cluster.local") - // ... + return pulumix.Apply2( + cmp.Chart.Status.Name(), + cmp.Chart.Status.Namespace(), + func(name, namespace *string) string { + return fmt.Sprintf("%s-postgresql.%s.svc.cluster.local", *name, *namespace) + }, + ) }deployments/pulumi/pkg/storage/component.go (2)
70-110: Logging or tracing of environment variable injection.
WhileGetEnvVars()properly constructs environment variables, consider adding Pulumi logs or checks to confirm these environment variables are correct at deploy-time. This improves observability and debugging when environment variables differ from expectations.
225-235: Check migration job completion or success state.
runMigrateJobpresumably runs a DB migration. Verify that job success/failure is handled before continuing. If the migrations fail, ensuring an appropriate rollback or warning might be beneficial.deployments/pulumi/pkg/config/config.go (1)
290-312: Validate “storage” configuration at design time.
If “storage” is not defined, the code errors out. Consider providing clearer instructions or logging about how to supply the storage config, to guide users properly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
deployments/pulumi/go.modis excluded by!**/*.moddeployments/pulumi/go.sumis excluded by!**/*.sum,!**/*.sumtools/k8s-stack/Pulumi.yamlis excluded by!**/*.yamltools/k8s-stack/go.modis excluded by!**/*.modtools/k8s-stack/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (25)
deployments/pulumi/main.go(1 hunks)deployments/pulumi/main_test.go(2 hunks)deployments/pulumi/pkg/api/component.go(1 hunks)deployments/pulumi/pkg/api/deployment.go(9 hunks)deployments/pulumi/pkg/api/ingress.go(3 hunks)deployments/pulumi/pkg/api/service.go(2 hunks)deployments/pulumi/pkg/component.go(1 hunks)deployments/pulumi/pkg/config/config.go(1 hunks)deployments/pulumi/pkg/devbox/component.go(1 hunks)deployments/pulumi/pkg/image.go(0 hunks)deployments/pulumi/pkg/storage/component.go(1 hunks)deployments/pulumi/pkg/storage/component_external.go(1 hunks)deployments/pulumi/pkg/storage/component_postgres.go(1 hunks)deployments/pulumi/pkg/storage/component_rds.go(1 hunks)deployments/pulumi/pkg/storage/migrate.go(2 hunks)deployments/pulumi/pkg/storage/setup_postgres.go(1 hunks)deployments/pulumi/pkg/storage/setup_rds.go(1 hunks)deployments/pulumi/pkg/utils/args.go(8 hunks)deployments/pulumi/pkg/utils/convert.go(1 hunks)deployments/pulumi/pkg/utils/sync.go(1 hunks)deployments/pulumi/pkg/worker/component.go(3 hunks)tools/k8s-stack/justfile(0 hunks)tools/k8s-stack/main.go(0 hunks)tools/k8s-stack/pkg/postgres.go(0 hunks)tools/k8s-stack/pkg/stack.go(0 hunks)
💤 Files with no reviewable changes (5)
- deployments/pulumi/pkg/image.go
- tools/k8s-stack/pkg/postgres.go
- tools/k8s-stack/justfile
- tools/k8s-stack/main.go
- tools/k8s-stack/pkg/stack.go
🔇 Additional comments (55)
deployments/pulumi/pkg/storage/migrate.go (5)
1-1: Package name update looks good.
Renaming the package tostorageclearly conveys its purpose, aligning with the refactoring goals and improving code organization.
11-14: Consider defensive checks for the component pointer.
Ifargs.componentis evernil, appending environment variables in subsequent lines may cause a runtime panic. Ensure this pointer is never nil or add checks before usage.
16-16: Function extraction is well-structured.
Splitting out therunMigrateJobfunction improves modularity and clarity. Just ensure unit testing coverage or integration tests cover this refactored logic.
39-41: Validate the image tagging logic.
Usingutils.GetImage(args.Tag)is cleaner, but ensureargs.Tagis set and valid. If empty or malformed, the logic may fall back to an unexpected image or trigger an error during deployment.
47-47: Confirm the implications of DeleteBeforeReplace.
pulumi.DeleteBeforeReplace(true)can cause downtime if the job is removed before being recreated. Ensure this aligns with the desired deployment guarantees.deployments/pulumi/pkg/utils/sync.go (1)
10-15: Be aware of blocking the Pulumi execution flow.By reading from a channel, you convert the asynchronous
pulumix.Applycall into a synchronous operation. This can degrade concurrency and reduce Pulumi’s ability to optimally plan and parallelize updates. Confirm that this blocking behavior is truly desired before proceeding.deployments/pulumi/pkg/api/service.go (1)
11-14: Validate non-nil deployment reference.
createServiceArgsallowsDeploymentto be nil, potentially causing runtime panics when accessing.Spec. Document or enforce non-nil usage to ensure resilience.deployments/pulumi/pkg/devbox/component.go (1)
19-23: Ensure optional fields are handled gracefully.
APIandStoragereferences may be nil if not provided. Verify that the rest of the codebase either checks for nil or ensures these components exist to avoid potential crashes.deployments/pulumi/main_test.go (3)
4-4: New JSON import looks good.
The addition ofencoding/jsonis appropriate for marshaling flexible configurations.
21-27: Validatetimeoutand nestedstoragekeys.
Storing an integer timeout and a nestedstorageconfiguration is good for clarity, but ensure the downstream code properly interprets these fields.
32-39: Serializing config values into JSON strings.
This approach effectively preserves typed data for Pulumi config entries. Make sure that the corresponding code properly unmarshals these JSON values when applying the configuration.deployments/pulumi/main.go (4)
5-5: Importing theconfigpackage.
No issues here. This lines up with the new config loading calls below.
10-10: Inline Pulumi run function is concise and idiomatic.
Defining the run logic in an anonymous function is a clean approach for Pulumi programs.
17-20: Creating the ledger component withcfg.ToInput().
This is an elegant way to pass all config parameters in a single typed struct. Just ensure thorough testing of the new input approach.
30-31: Returning the existing error is standard.
This error-handling pattern is straightforward; no further changes are required here.deployments/pulumi/pkg/api/component.go (8)
1-2: Newapipackage definition.
No issues in naming or file structure.
3-10: Imports align with usage.
All imported modules are relevant to Pulumi, Kubernetes, or supporting functionalities.
12-17:Componentstruct is well-defined.
Storing PulumiResourceStateplus the Deployment and Service references is a standard pattern.
26-29:NewComponentresource registration.
Registering the resource with"Formance:Ledger:API"is a clear naming choice. Adding a quick docstring forNewComponentmight be helpful for future maintainers.
33-40: Deployment creation flow.
Delegating tocreateDeploymentis neat. Ensure robust unit or integration tests exist for any logic internal to that function.
42-48: Service creation flow.
Similar to the deployment logic, confirming the correctness ofcreateServicevia tests will help avoid issues at runtime.
50-62: Conditional ingress creation with direct error return.
This approach is straightforward. The error check is immediate and consistent with the rest of the function.
64-69: Registering resource outputs.
Finalizing resource outputs ensures Pulumi can track the component’s resources properly.deployments/pulumi/pkg/api/ingress.go (2)
12-16: Validate that the provided Service is non-nil.
Ifargs.Serviceis ever nil, referencingargs.Service.Metadatawill fail. You may wish to add a guard or default to gracefully handle a nil service.
50-58: Confirm that TLS is optional and properly handled.
The code checks if the secret is nil or empty and returns no TLS configuration in that case, which appears correct. Ensure the host is still accessible without TLS, or clarify the expected behavior for a missing secret.deployments/pulumi/pkg/worker/component.go (6)
18-22: Consider adding a non-nil default forTerminationGracePeriodSeconds.
The default setsargs.TerminationGracePeriodSecondstonilwithout specifying an actual grace period. Verify whether this is desired behavior or if a numeric default (e.g., 30 seconds) is intended.
24-29:Servicefield is never assigned.
While yourComponentstruct includesService, it's currently unused inNewComponent. This might be intentional for future use or an oversight. Verify if you need to remove or utilize it.
31-35: ValidateDatabaseusage inComponentArgs.
The code callsargs.Database.GetEnvVars()at line 50 without anynilcheck. IfDatabaseis optional, consider a safety check to avoid potential runtime errors.
76-76: Termination grace period references.
Here we passargs.TerminationGracePeriodSecondsinto the deployment. Assess whether you need additional validation or a fallback if the field isnil, ensuring a predictable container shutdown behavior.
80-80: Good approach to fetching the container image.
Usingutils.GetImage(args.Tag)can help maintain consistency across various deployments. Ensure it covers all relevant tagging or fallback logic (like "latest" if empty).
92-98: Good error wrapping and consistent usage offmt.Errorf.
The error handling provides clarity about the deployment or output registration failure. Just make sure to continue this pattern across the entire codebase for uniform debugging.deployments/pulumi/pkg/storage/component_external.go (1)
67-67: Interface conformance check is helpful.
The_ DatabaseComponent = (*externalDatabaseComponent)(nil)ensures compile-time verification. Well done.deployments/pulumi/pkg/utils/args.go (2)
51-71: Default propagation forDebug,Tag, andNamespace.
TheSetDefaults()method properly sets them. Confirm that an emptyNamespaceorImagePullPolicyis acceptable in all contexts.
100-104: Consistent usage ofBoolToString.
You uniformly convert booleans to strings for environment variables. This is a neat approach. Keep the function well-documented to avoid confusion.Also applies to: 145-146, 165-166, 188-189, 206-207
deployments/pulumi/pkg/api/deployment.go (10)
1-1: Package renaming aligns with the new modular structure.Renaming the package from the former
pulumi_ledgertoapiis consistent with the reorganization described in the PR objectives, improving clarity and maintainability.
5-6: Imported packages for storage and utils appear relevant.These added imports from the
storageandutilspackages are correctly scoped, indicating a tighter integration of shared functionalities (like database setup and utility helpers) across modules.
12-12: Time-based features require caution around timezones and durations.By importing
"time", you are introducing support fortime.Durationin your code. Ensure that any user input forGracePeriodis validated thoroughly to avoid unexpected negative or extreme values.
15-17: GracePeriod conversion to time.Duration adds clarity.Switching from a string-based grace period to an explicit
time.Durationcan eliminate parsing errors at runtime, simplifying handling of wait intervals or timeouts. Make sure documentation and configuration reflect this new type.
27-55: Default-setting method is robust, but watch for zero vs. nil distinctions.The
SetDefaults()logic is consistent, ensuring every field has a fallback. However, fields likeBulkParallelandBulkMaxSizesilently default to zero, which could disable intended features if not clearly documented.
57-61: Encapsulating deployment arguments is a good practice.Defining
createDeploymentArgsconsolidates multiple argument sources (CommonArgs,Args, etc.) into one place, providing a cleaner contract for the deployment function.
63-72: Check for consistency betweenargs.Debugusage and the surrounding ecosystem.Using
utils.BoolToString(args.Debug)is valid for environment variables, but confirm that any downstream processes indeed accept a string "true"/"false" vs. requiring a numeric or different boolean format.
119-121: Grace period environmental variable may require user-friendly formatting.
time.Duration.String()yields a Go-style duration (e.g.,"1m0s"). Clarify in documentation and downstream consumers how this string is parsed or used in your application layer.
125-128: Database environment vars are appended conditionally.Appending
args.Database.GetEnvVars()combined withargs.GetOpenTelemetryEnvVars(...)suggests a clean approach to layering environment variables. Verify that no duplicates or conflicting environment variables result.
130-200: Deployment specification updated to 'ledger-api'.Renaming the application label and resource names to
"ledger-api"is consistent with the new nomenclature. The rest of the deployment spec appears correct, including readiness/liveness probes, though ensure that the chosenEnginehandles the new environment variable schema properly.deployments/pulumi/pkg/storage/component_rds.go (4)
1-10: New package and imports form a specialized RDS module.Defining a dedicated package for RDS components is a clean separation of concerns. Verify that the
rdsprovider version (v6) is aligned with any additional AWS resource dependencies throughout the codebase.
12-21: RDS component struct and associated fields are well-structured.Including both
ClusterandInstancereferences inRDSDatabaseComponentcaptures a typical multi-resource RDS setup. Ensure that you handle additional configurations (e.g., read replicas) if required in future expansions.
23-41: Exposing database connection details is straightforward, but watch for security.Methods like
GetPassword()andGetUsername()can inadvertently leak credentials if logged. Restrict usage of these methods to secure contexts, and consider more secure storage or retrieval mechanisms for production.
43-114: Ensure RDS cluster config matches AWS constraints.
"aurora-postgresql"with engine version"16"might not be fully GA in certain AWS regions. Confirm availability where you deploy.- The code properly checks for a non-empty subnet group, preventing misconfigurations.
- Having both cluster and instance creation in one place is convenient, but consider a small separation of concerns if instance classes or read replicas become more complex.
deployments/pulumi/pkg/storage/setup_rds.go (4)
1-8: Introducing a separate setup file improves organization.Centralizing RDS creation/use logic in
setup_rds.goclarifies the distinction between new cluster creation and referencing existing clusters.
55-67: UseExistingCluster defaulting.Setting empty defaults for
ClusterNameandMasterPasswordcan be beneficial, but ensure the calling code verifies these fields promptly. A silent empty default might lead to confusion if overlooked.
74-81: Graceful fallback for nested defaults is well-structured.
SetDefaults()is delegated to sub-structs (CreateClusterandUseCluster), preventing confusion and layering errors. The logic here is straightforward and easy to maintain.
83-122: Handling either existing or new RDS configuration is correct but watch for edge cases.
- Returning an error if both or neither are supplied is good validation.
- For existing clusters, be sure the
lookupClusterresult matches the region, engine, or other assumptions.- Consider expanding error messages to guide users on correct approaches if misconfiguration occurs.
deployments/pulumi/pkg/storage/component_postgres.go (2)
8-8: Questionable use of internal Pulumi package.
Importinggithub.com/pulumi/pulumi/sdk/v3/go/pulumi/internalsand relying oninternals.UnsafeAwaitOutputis risky because internal APIs may change without notice. Consider alternative approaches (e.g., leveraging available Pulumi outputs andApplymethods) to avoid potential instability.
86-109: Potential misuse ofUnsafeAwaitOutput.
Reliance oninternals.UnsafeAwaitOutputis an advanced pattern that can lead to blocking behaviors during preview and updates. Ensure it’s safely needed and well-documented; otherwise, consider using standardApplychaining.deployments/pulumi/pkg/storage/component.go (1)
170-179: Ensure mutual exclusivity of Postgres and RDS is enforced throughout.
The switch block correctly checks that only Postgres or RDS can be configured at once, but be sure that other parts of the code (e.g.,setupmethods) do not assume a default fallback if both are nil.
| func NewComponent(ctx *pulumi.Context, name string, args ComponentArgs, opts ...pulumi.ResourceOption) (*Component, error) { | ||
| cmp := &Component{} | ||
| err := ctx.RegisterComponentResource("Formance:Ledger:DevBox", name, cmp, opts...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| cmp.Deployment, err = appsv1.NewDeployment(ctx, "ledger-devbox", &appsv1.DeploymentArgs{ | ||
| Metadata: &metav1.ObjectMetaArgs{ | ||
| Namespace: args.Namespace.ToOutput(ctx.Context()).Untyped().(pulumi.StringOutput), | ||
| Labels: pulumi.StringMap{ | ||
| "com.formance.stack/app": pulumi.String("ledger-devbox"), | ||
| }, | ||
| }, | ||
| Spec: appsv1.DeploymentSpecArgs{ | ||
| Selector: &metav1.LabelSelectorArgs{ | ||
| MatchLabels: pulumi.StringMap{ | ||
| "com.formance.stack/app": pulumi.String("ledger-devbox"), | ||
| }, | ||
| }, | ||
| Template: &corev1.PodTemplateSpecArgs{ | ||
| Metadata: &metav1.ObjectMetaArgs{ | ||
| Labels: pulumi.StringMap{ | ||
| "com.formance.stack/app": pulumi.String("ledger-devbox"), | ||
| }, | ||
| }, | ||
| Spec: corev1.PodSpecArgs{ | ||
| TerminationGracePeriodSeconds: pulumi.IntPtr(0), | ||
| Containers: corev1.ContainerArray{ | ||
| corev1.ContainerArgs{ | ||
| Name: pulumi.String("ledger"), | ||
| Image: pulumi.String("alpine:3.21"), | ||
| ImagePullPolicy: pulumi.String("IfNotPresent"), | ||
| Args: pulumi.StringArray{ | ||
| pulumi.String("sh"), | ||
| pulumi.String("-c"), | ||
| pulumi.String(` | ||
| #!/bin/sh | ||
|
|
||
| apk update | ||
| apk add postgresql-client httpie bash | ||
|
|
||
| sleep infinity | ||
| `), | ||
| }, | ||
| Env: corev1.EnvVarArray{ | ||
| corev1.EnvVarArgs{ | ||
| Name: pulumi.String("POSTGRES_SERVICE_NAME"), | ||
| Value: args.Storage.Service.Metadata.Name(), | ||
| }, | ||
| corev1.EnvVarArgs{ | ||
| Name: pulumi.String("POSTGRES_USERNAME"), | ||
| ValueFrom: corev1.EnvVarSourceArgs{ | ||
| SecretKeyRef: &corev1.SecretKeySelectorArgs{ | ||
| Key: pulumi.String("username"), | ||
| Name: args.Storage.Credentials.Metadata.Name(), | ||
| }, | ||
| }, | ||
| }, | ||
| corev1.EnvVarArgs{ | ||
| Name: pulumi.String("POSTGRES_PASSWORD"), | ||
| ValueFrom: corev1.EnvVarSourceArgs{ | ||
| SecretKeyRef: &corev1.SecretKeySelectorArgs{ | ||
| Key: pulumi.String("password"), | ||
| Name: args.Storage.Credentials.Metadata.Name(), | ||
| }, | ||
| }, | ||
| }, | ||
| corev1.EnvVarArgs{ | ||
| Name: pulumi.String("PGPASSWORD"), | ||
| Value: pulumi.String("$(POSTGRES_PASSWORD)"), | ||
| }, | ||
| corev1.EnvVarArgs{ | ||
| Name: pulumi.String("API_SERVICE"), | ||
| Value: args.API.Service.Metadata.Name(), | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, pulumi.Parent(cmp)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("creating deployment: %w", err) | ||
| } | ||
|
|
||
| if err := ctx.RegisterResourceOutputs(cmp, pulumi.Map{}); err != nil { | ||
| return nil, fmt.Errorf("registering outputs: %w", err) | ||
| } | ||
|
|
||
| return cmp, nil | ||
| } |
There was a problem hiding this comment.
Avoid infinite sleep in production deployments.
The container runs indefinitely with sleep infinity, which is suitable for development scenarios but risky in production. Consider adding proper health checks, readiness probes, or a minimal process that terminates gracefully.
Proper error handling for external references.
Directly referencing args.API.Service and args.Storage might cause panics if misconfigured. Add checks or fail early if these fields are missing.
@@ -58,6 +58,11 @@ func NewComponent(ctx *pulumi.Context, name string, args ComponentArgs, opts ...
// Example guard check
if args.API == nil || args.API.Service == nil {
+ return nil, fmt.Errorf("API service is required but was not provided")
+ }
+ if args.Storage == nil || args.Storage.Service == nil || args.Storage.Credentials == nil {
+ return nil, fmt.Errorf("Storage configuration is incomplete or missing")
+ }
cmp.Deployment, err = appsv1.NewDeployment(ctx, "ledger-devbox", &appsv1.DeploymentArgs{
...📝 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.
| func NewComponent(ctx *pulumi.Context, name string, args ComponentArgs, opts ...pulumi.ResourceOption) (*Component, error) { | |
| cmp := &Component{} | |
| err := ctx.RegisterComponentResource("Formance:Ledger:DevBox", name, cmp, opts...) | |
| if err != nil { | |
| return nil, err | |
| } | |
| cmp.Deployment, err = appsv1.NewDeployment(ctx, "ledger-devbox", &appsv1.DeploymentArgs{ | |
| Metadata: &metav1.ObjectMetaArgs{ | |
| Namespace: args.Namespace.ToOutput(ctx.Context()).Untyped().(pulumi.StringOutput), | |
| Labels: pulumi.StringMap{ | |
| "com.formance.stack/app": pulumi.String("ledger-devbox"), | |
| }, | |
| }, | |
| Spec: appsv1.DeploymentSpecArgs{ | |
| Selector: &metav1.LabelSelectorArgs{ | |
| MatchLabels: pulumi.StringMap{ | |
| "com.formance.stack/app": pulumi.String("ledger-devbox"), | |
| }, | |
| }, | |
| Template: &corev1.PodTemplateSpecArgs{ | |
| Metadata: &metav1.ObjectMetaArgs{ | |
| Labels: pulumi.StringMap{ | |
| "com.formance.stack/app": pulumi.String("ledger-devbox"), | |
| }, | |
| }, | |
| Spec: corev1.PodSpecArgs{ | |
| TerminationGracePeriodSeconds: pulumi.IntPtr(0), | |
| Containers: corev1.ContainerArray{ | |
| corev1.ContainerArgs{ | |
| Name: pulumi.String("ledger"), | |
| Image: pulumi.String("alpine:3.21"), | |
| ImagePullPolicy: pulumi.String("IfNotPresent"), | |
| Args: pulumi.StringArray{ | |
| pulumi.String("sh"), | |
| pulumi.String("-c"), | |
| pulumi.String(` | |
| #!/bin/sh | |
| apk update | |
| apk add postgresql-client httpie bash | |
| sleep infinity | |
| `), | |
| }, | |
| Env: corev1.EnvVarArray{ | |
| corev1.EnvVarArgs{ | |
| Name: pulumi.String("POSTGRES_SERVICE_NAME"), | |
| Value: args.Storage.Service.Metadata.Name(), | |
| }, | |
| corev1.EnvVarArgs{ | |
| Name: pulumi.String("POSTGRES_USERNAME"), | |
| ValueFrom: corev1.EnvVarSourceArgs{ | |
| SecretKeyRef: &corev1.SecretKeySelectorArgs{ | |
| Key: pulumi.String("username"), | |
| Name: args.Storage.Credentials.Metadata.Name(), | |
| }, | |
| }, | |
| }, | |
| corev1.EnvVarArgs{ | |
| Name: pulumi.String("POSTGRES_PASSWORD"), | |
| ValueFrom: corev1.EnvVarSourceArgs{ | |
| SecretKeyRef: &corev1.SecretKeySelectorArgs{ | |
| Key: pulumi.String("password"), | |
| Name: args.Storage.Credentials.Metadata.Name(), | |
| }, | |
| }, | |
| }, | |
| corev1.EnvVarArgs{ | |
| Name: pulumi.String("PGPASSWORD"), | |
| Value: pulumi.String("$(POSTGRES_PASSWORD)"), | |
| }, | |
| corev1.EnvVarArgs{ | |
| Name: pulumi.String("API_SERVICE"), | |
| Value: args.API.Service.Metadata.Name(), | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, pulumi.Parent(cmp)) | |
| if err != nil { | |
| return nil, fmt.Errorf("creating deployment: %w", err) | |
| } | |
| if err := ctx.RegisterResourceOutputs(cmp, pulumi.Map{}); err != nil { | |
| return nil, fmt.Errorf("registering outputs: %w", err) | |
| } | |
| return cmp, nil | |
| } | |
| func NewComponent(ctx *pulumi.Context, name string, args ComponentArgs, opts ...pulumi.ResourceOption) (*Component, error) { | |
| cmp := &Component{} | |
| err := ctx.RegisterComponentResource("Formance:Ledger:DevBox", name, cmp, opts...) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Example guard check | |
| if args.API == nil || args.API.Service == nil { | |
| return nil, fmt.Errorf("API service is required but was not provided") | |
| } | |
| if args.Storage == nil || args.Storage.Service == nil || args.Storage.Credentials == nil { | |
| return nil, fmt.Errorf("Storage configuration is incomplete or missing") | |
| } | |
| cmp.Deployment, err = appsv1.NewDeployment(ctx, "ledger-devbox", &appsv1.DeploymentArgs{ | |
| Metadata: &metav1.ObjectMetaArgs{ | |
| Namespace: args.Namespace.ToOutput(ctx.Context()).Untyped().(pulumi.StringOutput), | |
| Labels: pulumi.StringMap{ | |
| "com.formance.stack/app": pulumi.String("ledger-devbox"), | |
| }, | |
| }, | |
| Spec: appsv1.DeploymentSpecArgs{ | |
| Selector: &metav1.LabelSelectorArgs{ | |
| MatchLabels: pulumi.StringMap{ | |
| "com.formance.stack/app": pulumi.String("ledger-devbox"), | |
| }, | |
| }, | |
| Template: &corev1.PodTemplateSpecArgs{ | |
| Metadata: &metav1.ObjectMetaArgs{ | |
| Labels: pulumi.StringMap{ | |
| "com.formance.stack/app": pulumi.String("ledger-devbox"), | |
| }, | |
| }, | |
| Spec: corev1.PodSpecArgs{ | |
| TerminationGracePeriodSeconds: pulumi.IntPtr(0), | |
| Containers: corev1.ContainerArray{ | |
| corev1.ContainerArgs{ | |
| Name: pulumi.String("ledger"), | |
| Image: pulumi.String("alpine:3.21"), | |
| ImagePullPolicy: pulumi.String("IfNotPresent"), | |
| Args: pulumi.StringArray{ | |
| pulumi.String("sh"), | |
| pulumi.String("-c"), | |
| pulumi.String(` | |
| #!/bin/sh | |
| apk update | |
| apk add postgresql-client httpie bash | |
| sleep infinity | |
| `), | |
| }, | |
| Env: corev1.EnvVarArray{ | |
| corev1.EnvVarArgs{ | |
| Name: pulumi.String("POSTGRES_SERVICE_NAME"), | |
| Value: args.Storage.Service.Metadata.Name(), | |
| }, | |
| corev1.EnvVarArgs{ | |
| Name: pulumi.String("POSTGRES_USERNAME"), | |
| ValueFrom: corev1.EnvVarSourceArgs{ | |
| SecretKeyRef: &corev1.SecretKeySelectorArgs{ | |
| Key: pulumi.String("username"), | |
| Name: args.Storage.Credentials.Metadata.Name(), | |
| }, | |
| }, | |
| }, | |
| corev1.EnvVarArgs{ | |
| Name: pulumi.String("POSTGRES_PASSWORD"), | |
| ValueFrom: corev1.EnvVarSourceArgs{ | |
| SecretKeyRef: &corev1.SecretKeySelectorArgs{ | |
| Key: pulumi.String("password"), | |
| Name: args.Storage.Credentials.Metadata.Name(), | |
| }, | |
| }, | |
| }, | |
| corev1.EnvVarArgs{ | |
| Name: pulumi.String("PGPASSWORD"), | |
| Value: pulumi.String("$(POSTGRES_PASSWORD)"), | |
| }, | |
| corev1.EnvVarArgs{ | |
| Name: pulumi.String("API_SERVICE"), | |
| Value: args.API.Service.Metadata.Name(), | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, pulumi.Parent(cmp)) | |
| if err != nil { | |
| return nil, fmt.Errorf("creating deployment: %w", err) | |
| } | |
| if err := ctx.RegisterResourceOutputs(cmp, pulumi.Map{}); err != nil { | |
| return nil, fmt.Errorf("registering outputs: %w", err) | |
| } | |
| return cmp, nil | |
| } |
| ctx.Export("namespace", cmp.Namespace.Metadata.Name()) | ||
| ctx.Export("api-deployment", cmp.API.Deployment.Metadata.Name()) | ||
| ctx.Export("api-service", cmp.API.Service.Metadata.Name().Elem()) | ||
| ctx.Export("worker-deployment", cmp.Worker.Deployment.Metadata.Name()) | ||
| ctx.Export("postgres-service", pulumi.Sprintf("%s", cmp.Storage.Service.Metadata.Name().Elem())) | ||
| ctx.Export("postgres-username", cmp.Storage.DatabaseComponent.GetUsername()) | ||
| ctx.Export("postgres-password", cmp.Storage.DatabaseComponent.GetPassword()) |
There was a problem hiding this comment.
Avoid exposing secrets in plaintext exports.
Exporting database credentials as plain text can pose a security risk. Consider using Pulumi secrets (e.g., ctx.Export("postgres-password", pulumi.ToSecret(...))) to avoid storing credentials unencrypted in state.
- ctx.Export("postgres-password", cmp.Storage.DatabaseComponent.GetPassword())
+ ctx.Export("postgres-password", pulumi.ToSecret(cmp.Storage.DatabaseComponent.GetPassword()))📝 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.
| ctx.Export("namespace", cmp.Namespace.Metadata.Name()) | |
| ctx.Export("api-deployment", cmp.API.Deployment.Metadata.Name()) | |
| ctx.Export("api-service", cmp.API.Service.Metadata.Name().Elem()) | |
| ctx.Export("worker-deployment", cmp.Worker.Deployment.Metadata.Name()) | |
| ctx.Export("postgres-service", pulumi.Sprintf("%s", cmp.Storage.Service.Metadata.Name().Elem())) | |
| ctx.Export("postgres-username", cmp.Storage.DatabaseComponent.GetUsername()) | |
| ctx.Export("postgres-password", cmp.Storage.DatabaseComponent.GetPassword()) | |
| ctx.Export("namespace", cmp.Namespace.Metadata.Name()) | |
| ctx.Export("api-deployment", cmp.API.Deployment.Metadata.Name()) | |
| ctx.Export("api-service", cmp.API.Service.Metadata.Name().Elem()) | |
| ctx.Export("worker-deployment", cmp.Worker.Deployment.Metadata.Name()) | |
| ctx.Export("postgres-service", pulumi.Sprintf("%s", cmp.Storage.Service.Metadata.Name().Elem())) | |
| ctx.Export("postgres-username", cmp.Storage.DatabaseComponent.GetUsername()) | |
| ctx.Export("postgres-password", pulumi.ToSecret(cmp.Storage.DatabaseComponent.GetPassword())) |
| if !install { | ||
| panic("uri must be provided if install is false") | ||
| } |
There was a problem hiding this comment.
Avoid panic in Pulumi code and return an error instead.
Panics abruptly stop execution and can cause difficult-to-diagnose failures in Pulumi programs. Consider returning an error to gracefully handle missing URIs when install is false.
- panic("uri must be provided if install is false")
+ return nil, fmt.Errorf("uri must be provided if install is false")📝 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.
| if !install { | |
| panic("uri must be provided if install is false") | |
| } | |
| if !install { | |
| return nil, fmt.Errorf("uri must be provided if install is false") | |
| } |
| func NewComponent(ctx *pulumi.Context, name string, args ComponentArgs, opts ...pulumi.ResourceOption) (*Component, error) { | ||
| cmp := &Component{} | ||
| err := ctx.RegisterComponentResource("Formance:Ledger", name, cmp, opts...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if args == nil { | ||
| args = &ComponentArgs{} | ||
| args.SetDefaults() | ||
|
|
||
| options := []pulumi.ResourceOption{ | ||
| pulumi.Parent(cmp), | ||
| } | ||
| args.setDefaults() | ||
|
|
||
| cmp.ServerDeployment, err = createAPIDeployment(ctx, cmp, args) | ||
| cmp.Namespace, err = corev1.NewNamespace(ctx, "namespace", &corev1.NamespaceArgs{ | ||
| Metadata: &metav1.ObjectMetaArgs{ | ||
| Name: args.Namespace. | ||
| ToOutput(ctx.Context()). | ||
| Untyped().(pulumi.StringOutput), | ||
| }, | ||
| }, options...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| cmp.WorkerDeployment, err = createWorkerDeployment(ctx, cmp, args) | ||
| options = append(options, pulumi.DependsOn([]pulumi.Resource{ | ||
| cmp.Namespace, | ||
| })) | ||
|
|
||
| cmp.Storage, err = storage.NewComponent(ctx, "storage", storage.ComponentArgs{ | ||
| CommonArgs: args.CommonArgs, | ||
| DatabaseArgs: args.Database, | ||
| }, options...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| _, err = newMigrationJob(ctx, cmp, args) | ||
| options = append(options, pulumi.DependsOn([]pulumi.Resource{ | ||
| // don't depend on storage since it includes migrations | ||
| // we just need the database to be up, migrations will be run in background | ||
| // we also need to have credentials ready for the API and Worker | ||
| cmp.Storage.DatabaseComponent, | ||
| cmp.Storage.Credentials, | ||
| cmp.Storage.Service, | ||
| })) | ||
|
|
||
| cmp.API, err = api.NewComponent(ctx, "api", api.ComponentArgs{ | ||
| CommonArgs: args.CommonArgs, | ||
| Args: args.API, | ||
| Storage: cmp.Storage, | ||
| Ingress: args.Ingress, | ||
| }, options...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| service, err := installService(ctx, cmp, *args) | ||
| cmp.Worker, err = worker.NewComponent(ctx, "worker", worker.ComponentArgs{ | ||
| CommonArgs: args.CommonArgs, | ||
| Args: args.Worker, | ||
| Database: cmp.Storage, | ||
| }, options...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| cmp.ServiceName = pulumix.Apply(service.Metadata.Name().ToStringPtrOutput(), func(name *string) string { | ||
| if name == nil { | ||
| return "" | ||
| } | ||
| return *name | ||
| }) | ||
| cmp.ServiceNamespace = pulumix.Apply(service.Metadata.Namespace().ToStringPtrOutput(), func(namespace *string) string { | ||
| if namespace == nil { | ||
| return "" | ||
| } | ||
| return *namespace | ||
| }) | ||
| cmp.ServicePort = pulumix.Val(8080) | ||
| cmp.ServiceInternalURL = pulumix.Apply(pulumi.Sprintf( | ||
| "http://%s.%s.svc.cluster.local:%d", | ||
| cmp.ServiceName, | ||
| cmp.ServiceNamespace, | ||
| cmp.ServicePort, | ||
| ), func(url string) string { | ||
| return url | ||
| }) | ||
|
|
||
| if args.API.Ingress != nil { | ||
| if _, err := installIngress(ctx, cmp, args); err != nil { | ||
| return nil, err | ||
| } | ||
| cmp.Devbox, err = devbox.NewComponent(ctx, "devbox", devbox.ComponentArgs{ | ||
| CommonArgs: args.CommonArgs, | ||
| API: cmp.API, | ||
| Storage: cmp.Storage, | ||
| }, options...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if err := ctx.RegisterResourceOutputs(cmp, pulumi.Map{ | ||
| "deployment-name": cmp.ServerDeployment.Metadata.Name(), | ||
| "service-name": cmp.ServiceName, | ||
| "service-namespace": cmp.ServiceNamespace, | ||
| "service-port": cmp.ServicePort, | ||
| "service-internal-url": cmp.ServiceInternalURL, | ||
| "deployment-name": cmp.API.Deployment.Metadata.Name(), | ||
| }); err != nil { | ||
| return nil, fmt.Errorf("registering resource outputs: %w", err) | ||
| } | ||
|
|
||
| return cmp, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Break down the NewComponent function for clarity.
This function handles namespace creation, storage setup, API, worker, and devbox components in a single block. Splitting it into logical subfunctions (e.g., createNamespace, createStorage, etc.) can improve readability and maintainability.
func NewComponent(ctx *pulumi.Context, name string, args ComponentArgs, opts ...pulumi.ResourceOption) (*Component, error) {
cmp := &Component{}
err := ctx.RegisterComponentResource("Formance:Ledger", name, cmp, opts...)
if err != nil {
return nil, err
}
args.SetDefaults()
- options := []pulumi.ResourceOption{
- pulumi.Parent(cmp),
- }
+ options := defaultOptions(cmp)
- cmp.Namespace, err = corev1.NewNamespace(ctx, "namespace", &corev1.NamespaceArgs{...}, options...)
- if err != nil {
- return nil, err
- }
+ ns, err := createNamespace(ctx, args, options)
+ if err != nil {
+ return nil, err
+ }
+ cmp.Namespace = ns
// ... more initialization calls
}Committable suggestion skipped: line range outside the PR's diff.
| type externalDatabaseComponent struct { | ||
| pulumi.ResourceState | ||
|
|
||
| Endpoint pulumix.Input[string] | ||
| Username pulumix.Input[string] | ||
| Password pulumix.Input[string] | ||
| Port pulumix.Input[int] | ||
| Options pulumix.Input[map[string]string] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider validating external database fields.
As these fields (Endpoint, Username, etc.) are critical for connectivity, you might want to ensure they aren't empty or missing. Provide user-friendly error messages if they fail validation.
| func (a *RDSDatabaseCreateArgs) SetDefaults() { | ||
| if a.MasterUsername == nil { | ||
| a.MasterUsername = pulumix.Val("") | ||
| } | ||
| a.MasterUsername = pulumix.Apply(a.MasterUsername, func(username string) string { | ||
| if username == "" { | ||
| return "root" | ||
| } | ||
| return username | ||
| }) | ||
| if a.MasterPassword == nil { | ||
| a.MasterPassword = pulumix.Val("") | ||
| } | ||
| a.MasterPassword = pulumix.Apply(a.MasterPassword, func(password string) string { | ||
| if password == "" { | ||
| return "password" | ||
| } | ||
| return password | ||
| }) | ||
| if a.PerformanceInsightsEnabled == nil { | ||
| a.PerformanceInsightsEnabled = pulumix.Val(false) | ||
| } | ||
| if a.SnapshotIdentifier == nil { | ||
| a.SnapshotIdentifier = pulumix.Val[*string](nil) | ||
| } | ||
| if a.InstanceClass == nil { | ||
| a.InstanceClass = pulumix.Val(rds.InstanceType_T3_Medium) | ||
| } | ||
| a.InstanceClass = pulumix.Apply(a.InstanceClass, func(instanceClass rds.InstanceType) rds.InstanceType { | ||
| if instanceClass == "" { | ||
| return rds.InstanceType_T3_Medium | ||
| } | ||
| return instanceClass | ||
| }) | ||
| } |
There was a problem hiding this comment.
Defaulting credentials to "root" / "password" can introduce security risks.
Although these defaults help with local experimentation, consider warning or preventing usage of weak credentials in production, e.g., via environment checks or validation logic.
- return "password"
+ // Encourage reading from a secure secret manager or environment variable.
+ // Alternatively, force users to supply a strong password at runtime.Committable suggestion skipped: line range outside the PR's diff.
| username := pulumix.Val("root") | ||
| password := pulumix.Val("password") |
There was a problem hiding this comment.
Hard-coded default credentials.
Storing default credentials in source code is insecure and may lead to unintentional usage in production environments. Externalize them via configuration, or generate random strong credentials at runtime.
| if !a.Install { | ||
| panic("uri must be provided if install is false") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using panic for missing configuration.
Panicking for an invalid config can disrupt Pulumi previews and partial deployments. Instead, return an error to gracefully stop the deployment.
-if !a.Install {
- panic("uri must be provided if install is false")
-}
+if !a.Install {
+ return nil, fmt.Errorf("uri must be provided if `install` is false")
+}📝 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.
| if !a.Install { | |
| panic("uri must be provided if install is false") | |
| } | |
| if !a.Install { | |
| return nil, fmt.Errorf("uri must be provided if `install` is false") | |
| } |
82e90ba to
e8deec2
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
deployments/pulumi/main.go (1)
27-28:⚠️ Potential issueAvoid exposing secrets in plaintext exports.
Exporting database credentials as plain text can pose a security risk. Consider using Pulumi secrets to avoid storing credentials unencrypted in state.
- ctx.Export("postgres-password", cmp.Storage.DatabaseComponent.GetPassword()) + ctx.Export("postgres-password", pulumi.ToSecret(cmp.Storage.DatabaseComponent.GetPassword()))deployments/pulumi/pkg/config/config.go (2)
99-101:⚠️ Potential issueAvoid using
panicfor missing configuration.Panicking for an invalid config can disrupt Pulumi previews and partial deployments. Instead, return an error to gracefully stop the deployment.
-if !a.Install { - panic("uri must be provided if install is false") -} +if !a.Install { + return nil, fmt.Errorf("uri must be provided if `install` is false") +}
89-106:⚠️ Potential issueUpdate return signature and handle errors properly in PostgresDatabase.toInput.
The method needs to return an error instead of panicking, but this requires updating the function signature and all calling code.
-func (a *PostgresDatabase) toInput() *storage.PostgresDatabaseArgs { +func (a *PostgresDatabase) toInput() (*storage.PostgresDatabaseArgs, error) { if a == nil { - return nil + return nil, nil } if a.URI != "" { return &storage.PostgresDatabaseArgs{ URI: pulumi.String(a.URI), - } + }, nil } if !a.Install { - panic("uri must be provided if install is false") + return nil, fmt.Errorf("uri must be provided if install is false") } return &storage.PostgresDatabaseArgs{ Install: pulumi.Bool(a.Install), - } + }, nil }Note: This change requires updating all callers of this method to handle the error return.
🧹 Nitpick comments (9)
deployments/pulumi/pkg/worker/component.go (3)
14-16: Consider a more descriptive struct name"name
Argsmay be ambiguous. ConsiderWorkerArgsorWorkerDeploymentArgsto improve clarity across the codebase."
76-76: Establish a default termination grace periodCurrently, the logic defaults to
nil, which may lead to unexpected behavior. Consider a safer default (e.g., 30 seconds).
96-100: Register deployment as an outputRegister the deployment in
RegisterResourceOutputsto enable referencing it from other resources or stacks.if err := ctx.RegisterResourceOutputs(cmp, pulumi.Map{ - + "deployment": cmp.Deployment, }); err != nil { return nil, fmt.Errorf("registering outputs: %w", err) }deployments/pulumi/pkg/api/service.go (1)
16-34: Consider making service name configurableThe hardcoded service name
"ledger"reduces flexibility. Accept a service name parameter increateServiceArgsto enable reuse.deployments/pulumi/pkg/devbox/component.go (1)
112-114: Register the deployment as an outputExposing
cmp.Deploymentallows other deployments or tasks to reference this devbox more easily.if err := ctx.RegisterResourceOutputs(cmp, pulumi.Map{ - + "deployment": cmp.Deployment, }); err != nil { return nil, fmt.Errorf("registering outputs: %w", err) }deployments/pulumi/pkg/api/component.go (1)
50-62: Consider enhancing error message for ingress creation.While the error handling for ingress creation is functional, it lacks the detailed error message formatting seen in the deployment and service creation.
- return nil, err + return nil, fmt.Errorf("creating ingress: %w", err)deployments/pulumi/pkg/storage/component.go (1)
205-207: Nitpick: Consider making the auto-generated Kubernetes Service name more descriptive.Using a generic name like
"storage"may cause collisions or confusion, especially if multiple services live in the same namespace. Consider adding a reference to the database type or context.deployments/pulumi/pkg/config/config.go (2)
287-340: Standardize error handling in the Load function.The current implementation has inconsistent error handling for missing configuration variables:
- "ingress" errors are ignored
- "storage" missing variables cause an error
- "api" and "monitoring" missing variables are ignored
This makes it unclear which configurations are required vs. optional.
Consider standardizing error handling by:
- Documenting which configurations are required vs. optional
- Using a consistent approach for required configurations
- Setting explicit defaults for all optional configurations
For example:
func Load(ctx *pulumi.Context) (*Config, error) { cfg := config.New(ctx, "") + // Optional: ingress configuration ingress := &Ingress{} if err := cfg.TryObject("ingress", ingress); err != nil { if !errors.Is(err, config.ErrMissingVar) { return nil, err } } + // Optional: timeout with default value of 60 timeout, err := config.TryInt(ctx, "timeout") if err != nil { if errors.Is(err, config.ErrMissingVar) { timeout = 60 } else { return nil, fmt.Errorf("error reading timeout: %w", err) } } + // Required: storage configuration storage := &Storage{} if err := config.GetObject(ctx, "storage", storage); err != nil { if !errors.Is(err, config.ErrMissingVar) { return nil, err } return nil, errors.New("storage not defined") } + // Optional: API configuration api := &API{} if err := config.GetObject(ctx, "api", api); err != nil { if !errors.Is(err, config.ErrMissingVar) { return nil, err } + // Set defaults for optional API configurations if needed + api = &API{ + // Default values + } } + // Optional: monitoring configuration otel := &Otel{} if err := config.GetObject(ctx, "monitoring", otel); err != nil { if !errors.Is(err, config.ErrMissingVar) { return nil, err } } return &Config{ Timeout: timeout, Common: Common{ Debug: config.GetBool(ctx, "debug"), Namespace: config.Get(ctx, "namespace"), Tag: config.Get(ctx, "version"), Otel: otel, }, Storage: storage, API: api, Ingress: ingress, }, nil }
306-312: Improve error message for missing storage configuration.The current error message is not descriptive enough to help users diagnose the problem.
storage := &Storage{} if err := config.GetObject(ctx, "storage", storage); err != nil { if !errors.Is(err, config.ErrMissingVar) { return nil, err } - return nil, errors.New("storage not defined") + return nil, errors.New("required configuration 'storage' is not defined in your Pulumi configuration") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
deployments/pulumi/go.modis excluded by!**/*.moddeployments/pulumi/go.sumis excluded by!**/*.sum,!**/*.sumtools/k8s-stack/Pulumi.yamlis excluded by!**/*.yamltools/k8s-stack/go.modis excluded by!**/*.modtools/k8s-stack/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (25)
deployments/pulumi/main.go(1 hunks)deployments/pulumi/main_test.go(2 hunks)deployments/pulumi/pkg/api/component.go(1 hunks)deployments/pulumi/pkg/api/deployment.go(9 hunks)deployments/pulumi/pkg/api/ingress.go(3 hunks)deployments/pulumi/pkg/api/service.go(2 hunks)deployments/pulumi/pkg/component.go(1 hunks)deployments/pulumi/pkg/config/config.go(1 hunks)deployments/pulumi/pkg/devbox/component.go(1 hunks)deployments/pulumi/pkg/image.go(0 hunks)deployments/pulumi/pkg/storage/component.go(1 hunks)deployments/pulumi/pkg/storage/component_external.go(1 hunks)deployments/pulumi/pkg/storage/component_postgres.go(1 hunks)deployments/pulumi/pkg/storage/component_rds.go(1 hunks)deployments/pulumi/pkg/storage/migrate.go(2 hunks)deployments/pulumi/pkg/storage/setup_postgres.go(1 hunks)deployments/pulumi/pkg/storage/setup_rds.go(1 hunks)deployments/pulumi/pkg/utils/args.go(8 hunks)deployments/pulumi/pkg/utils/convert.go(1 hunks)deployments/pulumi/pkg/utils/sync.go(1 hunks)deployments/pulumi/pkg/worker/component.go(3 hunks)tools/k8s-stack/justfile(0 hunks)tools/k8s-stack/main.go(0 hunks)tools/k8s-stack/pkg/postgres.go(0 hunks)tools/k8s-stack/pkg/stack.go(0 hunks)
💤 Files with no reviewable changes (5)
- deployments/pulumi/pkg/image.go
- tools/k8s-stack/pkg/postgres.go
- tools/k8s-stack/pkg/stack.go
- tools/k8s-stack/main.go
- tools/k8s-stack/justfile
🚧 Files skipped from review as they are similar to previous changes (4)
- deployments/pulumi/pkg/utils/sync.go
- deployments/pulumi/pkg/storage/component_external.go
- deployments/pulumi/pkg/storage/setup_postgres.go
- deployments/pulumi/pkg/utils/convert.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (55)
deployments/pulumi/pkg/devbox/component.go (2)
67-67: Avoid infinite sleep in productionThe container still uses
sleep infinity, which is risky. Implement a graceful shutdown or readiness approach.
21-22: Add guard checks for external referencesAccessing
args.API.Serviceandargs.Storage.*without validation can lead to runtime panics if they are nil.Also applies to: 71-99
deployments/pulumi/main_test.go (5)
4-4: Include json import to support config marshaling.The
encoding/jsonimport is correctly added to support the new configuration marshaling logic.
16-16: Good update to support complex configuration types.Changing the config type from
map[string]stringtomap[string]anyallows for more flexible and hierarchical configuration structures, which is a better design for complex deployment configurations.
21-28: Hierarchical configuration improves readability and organization.The nested configuration structure for storage settings is a good improvement over flat string values. It allows for better organization of related settings and makes the configuration more intuitive.
32-39: Well-structured configuration conversion with proper error handling.The code properly converts complex configuration objects to JSON strings, which is necessary for Pulumi's configuration system. The addition of a dynamically generated namespace with UUID is also a good practice for test isolation.
45-45: Simplified test configuration setup.The test now uses the generated config map directly without any intermediate PostgreSQL stack deployment, which simplifies the test structure.
deployments/pulumi/main.go (5)
5-5: Appropriate import for the new config package.The import of the dedicated config package indicates a good separation of concerns.
10-15: Simplified execution structure with proper error handling.The refactored code eliminates the separate
deployfunction and uses an inline anonymous function with proper error handling. This is cleaner and more concise.
17-20: Clean component initialization with explicit error handling.The component creation is well-structured with appropriate error checking.
22-26: Comprehensive export of deployment resources.Exporting these Kubernetes resources makes them available for reference in other stacks or for external tools to query. This is a good practice for infrastructure as code.
30-31: Consistent error handling.The function correctly returns the error from component creation if one occurred.
deployments/pulumi/pkg/api/component.go (6)
1-11: Well-structured package with appropriate imports.The API component is correctly placed in its own package with all necessary imports.
12-17: Clean component structure with embedded resource state.The Component struct properly embeds pulumi.ResourceState and includes the relevant Kubernetes resources as fields.
19-24: Well-designed arguments structure with dependency injection.The ComponentArgs struct effectively combines common arguments, component-specific arguments, storage dependency, and optional ingress configuration. This design allows for flexible component configuration.
26-41: Robust component registration with detailed error handling.The component registration and deployment creation include comprehensive error handling with descriptive error messages, making debugging easier.
42-48: Proper service creation with parent-child relationship.The service creation correctly uses the parent-child relationship for resource management and includes detailed error messages.
64-68: Proper resource output registration.The component correctly registers its outputs with the Pulumi context.
deployments/pulumi/pkg/storage/migrate.go (6)
1-1: Package name updated for better organization.Renaming from
pulumi_ledgertostorageimproves organization and follows Go's package naming conventions.
4-4: Added proper import for utilities package.The utils package import supports the use of common utilities across components.
11-14: Well-structured migration arguments type.The migrationArgs struct properly encapsulates common arguments and a reference to the storage component.
16-24: Improved environment variable configuration.The environment variables setup is now more organized and includes dynamic debugging configuration.
39-40: Better image handling with utility function.Using a dedicated
utils.GetImagefunction standardizes image reference generation across the codebase.
47-47: Added DeleteBeforeReplace for better resource management.The
DeleteBeforeReplaceoption ensures that the old job is deleted before creating a new one, which is important for jobs that must not run in parallel.deployments/pulumi/pkg/api/ingress.go (6)
1-1: Improved package organization with dedicated namespaces.The package renaming from
pulumi_ledgertoapienhances modularity by providing dedicated namespaces for different components.
12-16: Good type abstraction with clean, focused struct definition.The
IngressArgsstruct provides a clear encapsulation of ingress-related parameters, making the code more maintainable and the API more intuitive.
18-21: Well-structured composition using CommonArgs.The
createIngressArgsstruct effectively composes theutils.CommonArgswithIngressArgs, which promotes code reuse and maintains a consistent pattern across the codebase.
23-23: Improved function signature with resource options.The updated function signature includes variadic resource options, which is a good practice for Pulumi resources as it allows more flexibility when creating resources.
31-31: Direct field access enhances code clarity.Accessing fields directly from the structured arguments (e.g.,
args.Host,args.Service.Metadata.Name()) rather than through nested structures improves readability and makes the code easier to maintain.Also applies to: 37-37, 58-58
50-50: Elegant handling of optional TLS configuration.The use of
pulumix.Applyfor conditionally adding TLS configuration based on the secret value is a clean approach that avoids multiple nested conditionals.deployments/pulumi/pkg/component.go (9)
1-1: Package renaming enhances semantic clarity.Renaming from
pulumi_ledgertoledgermakes the package name more concise and reflective of its contents.
16-23: Well-structured ComponentArgs with logical organization.The
ComponentArgsstruct effectively encapsulates all necessary configuration for creating a ledger component, with clear separation of concerns between different subcomponents.
25-30: Robust defaults initialization with hierarchical approach.The
SetDefaultsmethod appropriately initializes defaults for nested components by calling their respectiveSetDefaultsmethods, ensuring consistent initialization throughout the component hierarchy.
32-40: Improved Component struct with clear component references.The
Componentstruct now directly references other components like API, Worker, Storage, etc., which makes the relationship between components explicit and easier to understand.
42-42: Function signature change enhances usability.Changing the function signature to accept
ComponentArgsby value rather than pointer simplifies usage and aligns with Go's idiomatic approach for small-to-medium sized structs.
78-85: Thoughtful dependency management with clear comments.The code includes a detailed comment explaining the dependency management strategy, specifically why it doesn't depend on the entire storage component but only on specific resources. This improves maintainability and helps future developers understand the design choices.
87-92: Consistent component creation pattern.The creation of the API component follows the same pattern as other components, passing the necessary arguments and dependencies, which maintains consistency across the codebase.
106-113: Added Devbox component enhances development experience.The inclusion of a Devbox component, which is conditionally added based on the provided arguments, demonstrates a thoughtful approach to supporting development workflows.
115-117: Simplified resource output registration.The resource output registration has been simplified to only include essential information (deployment name from the API component), which reduces unnecessary output and makes the output more focused.
deployments/pulumi/pkg/utils/args.go (5)
1-1: Appropriate package renaming for utility functions.Renaming the package from
pulumi_ledgertoutilsbetter reflects its role as a utility package that provides common functionality across the codebase.
43-49: Well-structured CommonArgs with focused scope.The
CommonArgsstruct effectively consolidates common parameters used across various components, enhancing consistency in argument handling throughout the codebase.
51-71: Exported SetDefaults method improves interoperability.The method has been renamed from
setDefaultstoSetDefaults, making it exportable and usable from other packages, which aligns with the modular design of the codebase.
73-73: Renamed method enhances consistency and visibility.The method has been renamed from
getOpenTelemetryEnvVarstoGetOpenTelemetryEnvVars, making it exportable and consistent with other exported methods in the codebase.
103-103: Consistent function naming with BoolToString.The references to
boolToStringhave been changed toBoolToString, ensuring consistency in function naming and visibility across the codebase.Also applies to: 145-145, 165-165, 188-188, 206-206
deployments/pulumi/pkg/api/deployment.go (8)
1-1: Package renaming enhances code organization.Renaming from
pulumi_ledgertoapiprovides a clearer namespace for API-specific components, improving the overall code organization.
15-15: Improved type safety with time.Duration.Changing
GracePeriodfrompulumix.Input[string]topulumix.Input[time.Duration]enhances type safety and makes the intent clearer, as it's explicitly a duration value rather than a generic string.Also applies to: 17-17
27-30: Enhanced default value handling for GracePeriod.Using
pulumix.Val(time.Duration(0))instead ofpulumi.String("0s")for the default grace period provides better type safety and removes the need for string parsing.
57-61: Well-structured createDeploymentArgs with clear composition.The
createDeploymentArgsstruct effectively composesutils.CommonArgsandArgsalong with aDatabasefield, providing a clean way to pass all necessary arguments to the deployment creation function.
63-63: Improved function signature with resource options.The updated function signature includes variadic resource options, which is a good practice for Pulumi resources as it allows more flexibility when creating resources.
119-122: Type-safe handling of duration values.The code now uses the native
String()method of thetime.Durationtype, which ensures that duration values are formatted correctly and consistently.
130-130: Consistent naming convention with "ledger-api".The deployment name and label values have been updated to use "ledger-api" consistently, which improves clarity and makes it easier to identify resources in the Kubernetes cluster.
Also applies to: 134-134, 141-141, 147-147, 154-155
155-155: Simplified image reference with utility function.Using
utils.GetImage(args.Tag)instead of constructing the image string directly reduces code duplication and centralizes the image naming logic, making it easier to maintain and update.deployments/pulumi/pkg/storage/component_rds.go (1)
62-65:Details
❓ Verification inconclusive
Consider verifying engine version and snapshot strategy.
Currently, the cluster is configured for Aurora PostgreSQL engine version 16 and is skipping the final snapshot. Please confirm that Aurora PostgreSQL 16 is GA or supported for your use case, and consider whether you truly want to skip the final snapshot in production environments (which may risk data loss).
You can confirm Aurora PostgreSQL engine version support using AWS documentation or by searching the web:
🌐 Web query:
Is Aurora PostgreSQL 16 general availability?💡 Result:
Amazon Aurora PostgreSQL 16 reached general availability (GA) on January 31, 2024, when AWS announced its support in all AWS Regions except China, including AWS GovCloud (US) Regions[2][4]. By March 4, 2024, it expanded to include the AWS China (Beijing) and China (Ningxia) Regions[4].
Key details about the GA release include:
- PostgreSQL 16 features:
- SQL/JSON constructors and identity functions
- Expanded query parallelism
pg_stat_ioview for I/O usage statistics[2][4]- Babelfish 4.0 integration: Added support for Full Text Search
CONTAINSfunction[2][4]- Deployment options: Available through the AWS Management Console, with upgrade paths for existing instances[2]
Subsequent updates introduced minor versions like 16.6 (December 2024), featuring:
- Reduced downtime for read replica upgrades
- Enhanced Global Database cross-region resiliency
- Performance improvements for large instances[7][8][10]
Aurora PostgreSQL 16 remains under standard support, with AWS actively backporting community fixes and adding Aurora-specific optimizations like faster COPY operations (16.2) and improved logical replication lag handling[3][10]. Users can upgrade existing clusters or create new instances via the RDS console.
Citations:
- 1: https://aws.amazon.com/about-aws/whats-new/2023/11/amazon-aurora-postgresql-16-rds-database-preview/
- 2: https://aws.amazon.com/about-aws/whats-new/2024/01/amazon-aurora-postgresql-compatible-edition-supports-version-16/
- 3: https://www.amazonaws.cn/en/new/2024/amazon-aurora-supports-postgresql-16-2-and-more-new-versions/
- 4: https://www.amazonaws.cn/en/new/2024/amazon-aurora-postgresql-compatible-edition-supports-postgresql-major-version-16/
- 5: https://aws.amazon.com/about-aws/whats-new/2025/01/amazon-aurora-postgresql-limitless-database-16-6/
- 6: https://repost.aws/questions/QU586x8RblToKSH-CBeF8B2g/aurora-end-of-standard-support-date-for-postgresql-15-4
- 7: https://www.amazonaws.cn/en/new/2025/amazon-aurora-now-supports-postgresql-16-6-15-10-14-15-13-18-and-12-22/
- 8: https://aws.amazon.com/about-aws/whats-new/2024/12/amazon-aurora-postgresql-supports-new-versions/
- 9: https://endoflife.date/amazon-rds-postgresql
- 10: https://docs.aws.amazon.com/AmazonRDS/latest/AuroraPostgreSQLReleaseNotes/AuroraPostgreSQL.Updates.html
Action Required: Revisit final snapshot configuration given production requirements
Aurora PostgreSQL 16 has been confirmed as generally available (GA) since January 31, 2024, so the engine version is valid. However, please review whether skipping the final snapshot (i.e.,
SkipFinalSnapshot: pulumi.Bool(true)) is intentional for your production deployments. Skipping the final snapshot can risk data loss if a recovery is needed; ensure that this decision aligns with your backup and disaster recovery strategy.
- Engine Version: Confirmed GA as of January 31, 2024.
- Snapshot Strategy: Double-check if skipping the final snapshot is appropriate for production or if adjustments are needed.
deployments/pulumi/pkg/storage/setup_rds.go (1)
20-37: Reiterating the critical security concern about hard-coded credentials.Storing default credentials in source code can pose security risks. If these defaults are ever used in a higher environment, it could compromise your database. Consider generating strong random credentials at runtime or requiring them to be provided via configuration or a secret store.
deployments/pulumi/pkg/storage/component_postgres.go (1)
64-65: Hard-coded default credentials pose a security risk.Using
"root"and"password"as defaults for username and password is insecure, and might inadvertently leak into production. Consider externalizing these values or generating them securely at runtime.
| type Component struct { | ||
| pulumi.ResourceState | ||
|
|
||
| Deployment *appsv1.Deployment | ||
| Service *corev1.Service | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unused Service field
The Service property on Component is never assigned. If you intend to expose a service, implement the creation logic; otherwise, remove this property.
| type ComponentArgs struct { | ||
| utils.CommonArgs | ||
| Args | ||
| Database *storage.Component | ||
| } |
There was a problem hiding this comment.
Check for nil Database
args.Database.GetEnvVars() can panic if Database is nil. Validate args.Database or return an informative error if it's missing.
+ if args.Database == nil {
+ return nil, fmt.Errorf("database component is required but missing")
+ }Committable suggestion skipped: line range outside the PR's diff.
| type createServiceArgs struct { | ||
| utils.CommonArgs | ||
| Deployment *v1.Deployment | ||
| } |
There was a problem hiding this comment.
Validate non-nil deployment
If args.Deployment is nil, referencing args.Deployment.Spec will panic. Add a guard check for a safer flow.
+ if args.Deployment == nil {
+ return nil, fmt.Errorf("deployment is required but missing")
+ }Committable suggestion skipped: line range outside the PR's diff.
| Namespace: pulumix.Val(c.Namespace), | ||
| Otel: c.Otel.ToInput(), | ||
| Tag: pulumix.Val(c.Tag), | ||
| ImagePullPolicy: pulumix.Val(c.ImagePullPolicy), | ||
| Debug: pulumix.Val(c.Debug), | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider validating the Common fields before building CommonArgs.
Fields like ImagePullPolicy might have specific allowed values (e.g., "Always", "IfNotPresent", "Never" in Kubernetes). Validating these values could prevent issues during deployment.
🏁 Script executed:
#!/bin/bash
# Find usage of ImagePullPolicy to determine allowed values
# Look for constants or validation of image pull policy in the codebase
rg -i "imagepullpolicy|image pull policy" -A 5 -B 5Length of output: 6301
Action Required: Implement Validation for Common Fields
After reviewing the code and searching for existing validation logic, it appears that the ImagePullPolicy field (and potentially other Common fields) is passed directly without ensuring the value is one of the allowed ones (e.g., "Always", "IfNotPresent", "Never"). There’s currently no validation in place in either deployments/pulumi/pkg/config/config.go or in the defaulting logic in deployments/pulumi/pkg/utils/args.go.
- Files Affected:
deployments/pulumi/pkg/config/config.go(around lines 261-266)deployments/pulumi/pkg/utils/args.go(in theSetDefaultsfunction)
Recommendation:
Introduce explicit validation for the Common fields before constructing the CommonArgs. For example, add a check that confirms c.ImagePullPolicy is one of the allowed Kubernetes values ("Always", "IfNotPresent", "Never") so that invalid configurations are caught early in the deployment process.
| func (s Storage) toInput() storage.DatabaseArgs { | ||
| return storage.DatabaseArgs{ | ||
| Postgres: s.Postgres.toInput(), | ||
| RDS: s.RDS.toInput(), | ||
| ConnectivityDatabaseArgs: s.Connectivity.toInput(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Update Storage.toInput to handle errors from PostgresDatabase.toInput.
If you implement the error handling in PostgresDatabase.toInput, you'll need to update this method to propagate errors.
-func (s Storage) toInput() storage.DatabaseArgs {
+func (s Storage) toInput() (storage.DatabaseArgs, error) {
+ postgres, err := s.Postgres.toInput()
+ if err != nil {
+ return storage.DatabaseArgs{}, err
+ }
+
return storage.DatabaseArgs{
- Postgres: s.Postgres.toInput(),
+ Postgres: postgres,
RDS: s.RDS.toInput(),
ConnectivityDatabaseArgs: s.Connectivity.toInput(),
- }
+ }, nil
}📝 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.
| func (s Storage) toInput() storage.DatabaseArgs { | |
| return storage.DatabaseArgs{ | |
| Postgres: s.Postgres.toInput(), | |
| RDS: s.RDS.toInput(), | |
| ConnectivityDatabaseArgs: s.Connectivity.toInput(), | |
| } | |
| } | |
| func (s Storage) toInput() (storage.DatabaseArgs, error) { | |
| postgres, err := s.Postgres.toInput() | |
| if err != nil { | |
| return storage.DatabaseArgs{}, err | |
| } | |
| return storage.DatabaseArgs{ | |
| Postgres: postgres, | |
| RDS: s.RDS.toInput(), | |
| ConnectivityDatabaseArgs: s.Connectivity.toInput(), | |
| }, nil | |
| } |
| func (cfg Config) ToInput() pulumi_ledger.ComponentArgs { | ||
| return pulumi_ledger.ComponentArgs{ | ||
| CommonArgs: cfg.Common.toInput(), | ||
| Database: cfg.Storage.toInput(), | ||
| API: cfg.API.toInput(), | ||
| Timeout: pulumix.Val(cfg.Timeout), | ||
| Ingress: cfg.Ingress.toInput(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Update Config.ToInput to handle errors from Storage.toInput.
If you implement error handling in the Storage.toInput method, you'll need to update this method to handle those errors.
-func (cfg Config) ToInput() pulumi_ledger.ComponentArgs {
+func (cfg Config) ToInput() (pulumi_ledger.ComponentArgs, error) {
+ database, err := cfg.Storage.toInput()
+ if err != nil {
+ return pulumi_ledger.ComponentArgs{}, err
+ }
+
return pulumi_ledger.ComponentArgs{
CommonArgs: cfg.Common.toInput(),
- Database: cfg.Storage.toInput(),
+ Database: database,
API: cfg.API.toInput(),
Timeout: pulumix.Val(cfg.Timeout),
Ingress: cfg.Ingress.toInput(),
- }
+ }, nil
}📝 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.
| func (cfg Config) ToInput() pulumi_ledger.ComponentArgs { | |
| return pulumi_ledger.ComponentArgs{ | |
| CommonArgs: cfg.Common.toInput(), | |
| Database: cfg.Storage.toInput(), | |
| API: cfg.API.toInput(), | |
| Timeout: pulumix.Val(cfg.Timeout), | |
| Ingress: cfg.Ingress.toInput(), | |
| } | |
| } | |
| func (cfg Config) ToInput() (pulumi_ledger.ComponentArgs, error) { | |
| database, err := cfg.Storage.toInput() | |
| if err != nil { | |
| return pulumi_ledger.ComponentArgs{}, err | |
| } | |
| return pulumi_ledger.ComponentArgs{ | |
| CommonArgs: cfg.Common.toInput(), | |
| Database: database, | |
| API: cfg.API.toInput(), | |
| Timeout: pulumix.Val(cfg.Timeout), | |
| Ingress: cfg.Ingress.toInput(), | |
| }, nil | |
| } |
| func (a *RDSDatabase) toInput() *storage.RDSDatabaseArgs { | ||
| if a == nil { | ||
| return nil | ||
| } | ||
|
|
||
| return &storage.RDSDatabaseArgs{ | ||
| CreateCluster: a.CreateCluster.toInput(), | ||
| UseCluster: a.UseCluster.toInput(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Add nil checks for nested objects to prevent panic.
The toInput method for RDSDatabase needs nil checks for a.CreateCluster and a.UseCluster before calling their methods.
Apply this fix to prevent nil pointer dereferences:
func (a *RDSDatabase) toInput() *storage.RDSDatabaseArgs {
if a == nil {
return nil
}
return &storage.RDSDatabaseArgs{
- CreateCluster: a.CreateCluster.toInput(),
- UseCluster: a.UseCluster.toInput(),
+ CreateCluster: a.CreateCluster != nil ? a.CreateCluster.toInput() : nil,
+ UseCluster: a.UseCluster != nil ? a.UseCluster.toInput() : nil,
}
}Committable suggestion skipped: line range outside the PR's diff.
| func (t *OtelTraces) toInput() *utils.OtelTracesArgs { | ||
| if t == nil { | ||
| return nil | ||
| } | ||
| return &utils.OtelTracesArgs{ | ||
| OtelTracesBatch: pulumix.Val(t.Batch), | ||
| OtelTracesExporterFlag: pulumix.Val(t.ExporterFlag), | ||
| OtelTracesExporterJaegerEndpoint: pulumix.Val(t.Jaeger.Endpoint), | ||
| OtelTracesExporterJaegerUser: pulumix.Val(t.Jaeger.User), | ||
| OtelTracesExporterJaegerPassword: pulumix.Val(t.Jaeger.Password), | ||
| OtelTracesExporterOTLPMode: pulumix.Val(t.OTLP.Mode), | ||
| OtelTracesExporterOTLPEndpoint: pulumix.Val(t.OTLP.Endpoint), | ||
| OtelTracesExporterOTLPInsecure: pulumix.Val(t.OTLP.Insecure), | ||
| } | ||
| } |
There was a problem hiding this comment.
Add nil checks for Jaeger and OTLP to prevent nil pointer dereference.
The function doesn't verify if t.Jaeger or t.OTLP are nil before accessing their fields, which could cause a panic if either is nil.
Apply this safer implementation:
func (t *OtelTraces) toInput() *utils.OtelTracesArgs {
if t == nil {
return nil
}
+ args := &utils.OtelTracesArgs{
+ OtelTracesBatch: pulumix.Val(t.Batch),
+ OtelTracesExporterFlag: pulumix.Val(t.ExporterFlag),
+ }
+
+ if t.Jaeger != nil {
+ args.OtelTracesExporterJaegerEndpoint = pulumix.Val(t.Jaeger.Endpoint)
+ args.OtelTracesExporterJaegerUser = pulumix.Val(t.Jaeger.User)
+ args.OtelTracesExporterJaegerPassword = pulumix.Val(t.Jaeger.Password)
+ }
+
+ if t.OTLP != nil {
+ args.OtelTracesExporterOTLPMode = pulumix.Val(t.OTLP.Mode)
+ args.OtelTracesExporterOTLPEndpoint = pulumix.Val(t.OTLP.Endpoint)
+ args.OtelTracesExporterOTLPInsecure = pulumix.Val(t.OTLP.Insecure)
+ }
+
+ return args
- return &utils.OtelTracesArgs{
- OtelTracesBatch: pulumix.Val(t.Batch),
- OtelTracesExporterFlag: pulumix.Val(t.ExporterFlag),
- OtelTracesExporterJaegerEndpoint: pulumix.Val(t.Jaeger.Endpoint),
- OtelTracesExporterJaegerUser: pulumix.Val(t.Jaeger.User),
- OtelTracesExporterJaegerPassword: pulumix.Val(t.Jaeger.Password),
- OtelTracesExporterOTLPMode: pulumix.Val(t.OTLP.Mode),
- OtelTracesExporterOTLPEndpoint: pulumix.Val(t.OTLP.Endpoint),
- OtelTracesExporterOTLPInsecure: pulumix.Val(t.OTLP.Insecure),
- }
}📝 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.
| func (t *OtelTraces) toInput() *utils.OtelTracesArgs { | |
| if t == nil { | |
| return nil | |
| } | |
| return &utils.OtelTracesArgs{ | |
| OtelTracesBatch: pulumix.Val(t.Batch), | |
| OtelTracesExporterFlag: pulumix.Val(t.ExporterFlag), | |
| OtelTracesExporterJaegerEndpoint: pulumix.Val(t.Jaeger.Endpoint), | |
| OtelTracesExporterJaegerUser: pulumix.Val(t.Jaeger.User), | |
| OtelTracesExporterJaegerPassword: pulumix.Val(t.Jaeger.Password), | |
| OtelTracesExporterOTLPMode: pulumix.Val(t.OTLP.Mode), | |
| OtelTracesExporterOTLPEndpoint: pulumix.Val(t.OTLP.Endpoint), | |
| OtelTracesExporterOTLPInsecure: pulumix.Val(t.OTLP.Insecure), | |
| } | |
| } | |
| func (t *OtelTraces) toInput() *utils.OtelTracesArgs { | |
| if t == nil { | |
| return nil | |
| } | |
| args := &utils.OtelTracesArgs{ | |
| OtelTracesBatch: pulumix.Val(t.Batch), | |
| OtelTracesExporterFlag: pulumix.Val(t.ExporterFlag), | |
| } | |
| if t.Jaeger != nil { | |
| args.OtelTracesExporterJaegerEndpoint = pulumix.Val(t.Jaeger.Endpoint) | |
| args.OtelTracesExporterJaegerUser = pulumix.Val(t.Jaeger.User) | |
| args.OtelTracesExporterJaegerPassword = pulumix.Val(t.Jaeger.Password) | |
| } | |
| if t.OTLP != nil { | |
| args.OtelTracesExporterOTLPMode = pulumix.Val(t.OTLP.Mode) | |
| args.OtelTracesExporterOTLPEndpoint = pulumix.Val(t.OTLP.Endpoint) | |
| args.OtelTracesExporterOTLPInsecure = pulumix.Val(t.OTLP.Insecure) | |
| } | |
| return args | |
| } |
| func (m *OtelMetrics) toInput() *utils.OtelMetricsArgs { | ||
| if m == nil { | ||
| return nil | ||
| } | ||
| return &utils.OtelMetricsArgs{ | ||
| OtelMetricsExporterPushInterval: pulumix.Val(m.PushInterval), | ||
| OtelMetricsRuntime: pulumix.Val(m.Runtime), | ||
| OtelMetricsRuntimeMinimumReadMemStatsInterval: pulumix.Val(m.RuntimeMinimumReadMemStatsInterval), | ||
| OtelMetricsExporter: pulumix.Val(m.Exporter), | ||
| OtelMetricsKeepInMemory: pulumix.Val(m.KeepInMemory), | ||
| OtelMetricsExporterOTLPMode: pulumix.Val(m.OtelMetricsOTLP.Mode), | ||
| OtelMetricsExporterOTLPEndpoint: pulumix.Val(m.OtelMetricsOTLP.Endpoint), | ||
| OtelMetricsExporterOTLPInsecure: pulumix.Val(m.OtelMetricsOTLP.Insecure), | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix potential nil pointer dereference in OtelMetrics.
Similar to the OtelTraces issue, the function should check if m.OtelMetricsOTLP is nil before accessing its fields.
Apply this safer implementation:
func (m *OtelMetrics) toInput() *utils.OtelMetricsArgs {
if m == nil {
return nil
}
- return &utils.OtelMetricsArgs{
- OtelMetricsExporterPushInterval: pulumix.Val(m.PushInterval),
- OtelMetricsRuntime: pulumix.Val(m.Runtime),
- OtelMetricsRuntimeMinimumReadMemStatsInterval: pulumix.Val(m.RuntimeMinimumReadMemStatsInterval),
- OtelMetricsExporter: pulumix.Val(m.Exporter),
- OtelMetricsKeepInMemory: pulumix.Val(m.KeepInMemory),
- OtelMetricsExporterOTLPMode: pulumix.Val(m.OtelMetricsOTLP.Mode),
- OtelMetricsExporterOTLPEndpoint: pulumix.Val(m.OtelMetricsOTLP.Endpoint),
- OtelMetricsExporterOTLPInsecure: pulumix.Val(m.OtelMetricsOTLP.Insecure),
- }
+ args := &utils.OtelMetricsArgs{
+ OtelMetricsExporterPushInterval: pulumix.Val(m.PushInterval),
+ OtelMetricsRuntime: pulumix.Val(m.Runtime),
+ OtelMetricsRuntimeMinimumReadMemStatsInterval: pulumix.Val(m.RuntimeMinimumReadMemStatsInterval),
+ OtelMetricsExporter: pulumix.Val(m.Exporter),
+ OtelMetricsKeepInMemory: pulumix.Val(m.KeepInMemory),
+ }
+
+ if m.OtelMetricsOTLP != nil {
+ args.OtelMetricsExporterOTLPMode = pulumix.Val(m.OtelMetricsOTLP.Mode)
+ args.OtelMetricsExporterOTLPEndpoint = pulumix.Val(m.OtelMetricsOTLP.Endpoint)
+ args.OtelMetricsExporterOTLPInsecure = pulumix.Val(m.OtelMetricsOTLP.Insecure)
+ }
+
+ return args
}📝 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.
| func (m *OtelMetrics) toInput() *utils.OtelMetricsArgs { | |
| if m == nil { | |
| return nil | |
| } | |
| return &utils.OtelMetricsArgs{ | |
| OtelMetricsExporterPushInterval: pulumix.Val(m.PushInterval), | |
| OtelMetricsRuntime: pulumix.Val(m.Runtime), | |
| OtelMetricsRuntimeMinimumReadMemStatsInterval: pulumix.Val(m.RuntimeMinimumReadMemStatsInterval), | |
| OtelMetricsExporter: pulumix.Val(m.Exporter), | |
| OtelMetricsKeepInMemory: pulumix.Val(m.KeepInMemory), | |
| OtelMetricsExporterOTLPMode: pulumix.Val(m.OtelMetricsOTLP.Mode), | |
| OtelMetricsExporterOTLPEndpoint: pulumix.Val(m.OtelMetricsOTLP.Endpoint), | |
| OtelMetricsExporterOTLPInsecure: pulumix.Val(m.OtelMetricsOTLP.Insecure), | |
| } | |
| } | |
| func (m *OtelMetrics) toInput() *utils.OtelMetricsArgs { | |
| if m == nil { | |
| return nil | |
| } | |
| args := &utils.OtelMetricsArgs{ | |
| OtelMetricsExporterPushInterval: pulumix.Val(m.PushInterval), | |
| OtelMetricsRuntime: pulumix.Val(m.Runtime), | |
| OtelMetricsRuntimeMinimumReadMemStatsInterval: pulumix.Val(m.RuntimeMinimumReadMemStatsInterval), | |
| OtelMetricsExporter: pulumix.Val(m.Exporter), | |
| OtelMetricsKeepInMemory: pulumix.Val(m.KeepInMemory), | |
| } | |
| if m.OtelMetricsOTLP != nil { | |
| args.OtelMetricsExporterOTLPMode = pulumix.Val(m.OtelMetricsOTLP.Mode) | |
| args.OtelMetricsExporterOTLPEndpoint = pulumix.Val(m.OtelMetricsOTLP.Endpoint) | |
| args.OtelMetricsExporterOTLPInsecure = pulumix.Val(m.OtelMetricsOTLP.Insecure) | |
| } | |
| return args | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
deployments/pulumi/pkg/devbox/component.go (2)
52-67: 🛠️ Refactor suggestionReplace
sleep infinitywith a proper long-running process.The container runs indefinitely with
sleep infinity, which is suitable for development scenarios but can lead to issues in production: no proper health checks, harder to monitor, and no graceful termination handling.Consider replacing with a more robust approach:
Args: pulumi.StringArray{ pulumi.String("sh"), pulumi.String("-c"), pulumi.String(` #!/bin/sh apk update apk add postgresql-client httpie bash - - sleep infinity + + # Run a simple health check service that can respond to signals + echo "DevBox container started. Running monitoring service." + trap "echo 'Shutting down gracefully'; exit 0" TERM INT + # Simple HTTP service that returns 200 OK for health checks + while true; do + sleep 3600 & wait $! + done `), },
23-111:⚠️ Potential issueAdd necessary null reference checks in the
NewComponentfunction.The function directly accesses
args.Storage.Service.Metadata.Name()andargs.Storage.Credentials.Metadata.Name()without validating if these references exist. This could lead to panic errors during deployment if any of these components are nil.Add validation checks at the beginning of the function:
func NewComponent(ctx *pulumi.Context, name string, args ComponentArgs, opts ...pulumi.ResourceOption) (*Component, error) { cmp := &Component{} err := ctx.RegisterComponentResource("Formance:Ledger:DevBox", name, cmp, opts...) if err != nil { return nil, err } + if args.Storage == nil { + return nil, fmt.Errorf("storage is required but was not provided") + } + if args.Storage.Service == nil || args.Storage.Service.Metadata == nil { + return nil, fmt.Errorf("storage service is required but was not properly configured") + } + if args.Storage.Credentials == nil || args.Storage.Credentials.Metadata == nil { + return nil, fmt.Errorf("storage credentials are required but were not properly configured") + } cmp.Deployment, err = appsv1.NewDeployment(ctx, "ledger-devbox", &appsv1.DeploymentArgs{ // ...deployments/pulumi/pkg/worker/component.go (2)
24-29:⚠️ Potential issueService property is declared but never assigned.
The
Componentstruct declares aServicefield, but this field is never assigned in theNewComponentfunction, making it always nil.Either implement the service creation or remove the field:
type Component struct { pulumi.ResourceState Deployment *appsv1.Deployment - Service *corev1.Service }
50-53:⚠️ Potential issueAdd nil check for Database before accessing it.
The function calls
args.Database.GetEnvVars()without checking ifargs.Databaseis nil, which could cause a panic.-envVars = append(envVars, args.Database.GetEnvVars()...) +if args.Database == nil { + return nil, fmt.Errorf("database component is required but was not provided") +} +envVars = append(envVars, args.Database.GetEnvVars()...)
🧹 Nitpick comments (19)
deployments/pulumi/justfile (1)
4-5: Consider adding error handling and verification steps.While the commands should work as expected, consider adding:
- Error checks to verify the schema file was generated successfully
- Validation that the Docker container exists/can be pulled
- A check that the output files are created with proper content
update-doc: go run tools/main.go schema > docs/schema.json - cat docs/schema.json | docker run --rm -i elisiariocouto/jsonschema-markdown - > docs/schema.md + [ -s docs/schema.json ] || { echo "Schema generation failed"; exit 1; } + cat docs/schema.json | docker run --rm -i elisiariocouto/jsonschema-markdown - > docs/schema.md + [ -s docs/schema.md ] || { echo "Markdown conversion failed"; exit 1; } + echo "Documentation updated successfully"deployments/pulumi/tools/main.go (2)
29-32: Remove or uncomment the commented configuration parameter.There's a commented configuration parameter
//DoNotReference: truewhich should either be removed if not needed or uncommented if it's meant to be used.func printSchema(_ *cobra.Command, _ []string) error { reflector := jsonschema.Reflector{ - //DoNotReference: true, RequiredFromJSONSchemaTags: true, }
38-39: Add explicit error handling for JSON encoding.The JSON encoding operation could fail but there's no explicit error handling for it. Even though
NewEncoder.Encodereturns the error which is then returned by the function, it would be clearer to check for the error explicitly before returning.- return json.NewEncoder(os.Stdout).Encode(schema) + err := json.NewEncoder(os.Stdout).Encode(schema) + if err != nil { + return fmt.Errorf("failed to encode schema: %w", err) + } + return nildeployments/pulumi/pkg/utils/args_common.go (2)
20-29: Simplify the Tag default logic.The Tag default logic is more complex than necessary. You can simplify the conditional logic while maintaining the same behavior.
if args.Tag == nil { args.Tag = pulumi.String("latest") } else { - args.Tag = pulumix.Apply(args.Tag, func(tag string) string { - if tag == "" { - return "latest" - } - return tag - }) + args.Tag = pulumix.Apply(args.Tag, func(tag string) string { + if tag == "" { + return "latest" + } + return tag + }) }
30-32: Consider using a specific default value for ImagePullPolicy.The current default for
ImagePullPolicyis an empty string, which may not be explicit enough. Consider using a Kubernetes standard value like "IfNotPresent" or documenting the behavior of an empty string.if args.ImagePullPolicy == nil { - args.ImagePullPolicy = pulumi.String("") + args.ImagePullPolicy = pulumi.String("IfNotPresent") // Using Kubernetes default }deployments/pulumi/pkg/devbox/component.go (1)
49-51: Add resource limits to the container spec.The container doesn't have any CPU or memory limits specified, which could potentially lead to resource contention or excessive resource usage in the cluster.
Add resource limits to the container:
Spec: corev1.PodSpecArgs{ TerminationGracePeriodSeconds: pulumi.IntPtr(0), + Resources: &corev1.ResourceRequirementsArgs{ + Limits: pulumi.StringMap{ + "cpu": pulumi.String("100m"), + "memory": pulumi.String("128Mi"), + }, + Requests: pulumi.StringMap{ + "cpu": pulumi.String("50m"), + "memory": pulumi.String("64Mi"), + }, + }, Containers: corev1.ContainerArray{ // ...deployments/pulumi/pkg/utils/args_otel.go (2)
57-68: Enhance resource attributes handling to prevent potential injection.The current implementation for building the resource attributes string directly concatenates keys and values, which could lead to unexpected behavior if the attributes contain commas or equal signs.
Use proper escaping or encoding for special characters:
Value: pulumix.Apply(args.ResourceAttributes, func(rawResourceAttributes map[string]string) string { - ret := "" - keys := collectionutils.Keys(rawResourceAttributes) - slices.Sort(keys) - for _, key := range keys { - ret += key + "=" + rawResourceAttributes[key] + "," - } - if len(ret) > 0 { - ret = ret[:len(ret)-1] - } - return ret + var parts []string + keys := collectionutils.Keys(rawResourceAttributes) + slices.Sort(keys) + for _, key := range keys { + // Escape commas and equal signs in both key and value + escapedKey := strings.ReplaceAll(strings.ReplaceAll(key, ",", "\\,"), "=", "\\=") + escapedValue := strings.ReplaceAll(strings.ReplaceAll(rawResourceAttributes[key], ",", "\\,"), "=", "\\=") + parts = append(parts, escapedKey+"="+escapedValue) + } + return strings.Join(parts, ",") }).Untyped().(pulumi.StringOutput),Don't forget to add the import for the
stringspackage at the top:import ( "context" "github.com/formancehq/go-libs/v2/collectionutils" corev1 "github.com/pulumi/pulumi-kubernetes/sdk/v4/go/kubernetes/core/v1" "github.com/pulumi/pulumi/sdk/v3/go/pulumi" "github.com/pulumi/pulumi/sdk/v3/go/pulumix" + "strings" "slices" "time" )
126-132: Refactor duplicate time.Duration handling logic into a helper function.The code for handling time.Duration pointers is duplicated in two places, which violates the DRY principle.
Extract the logic into a helper function:
// Add at the end of the file +func durationToString(duration *time.Duration) string { + if duration == nil { + return "" + } + return duration.String() +} // Then change the usage: Value: pulumix.Apply(metrics.OtelMetricsExporterPushInterval, func(pushInterval *time.Duration) string { - if pushInterval == nil { - return "" - } - return pushInterval.String() + return durationToString(pushInterval) }).Untyped().(pulumi.StringOutput), // And later: Value: pulumix.Apply(metrics.OtelMetricsRuntimeMinimumReadMemStatsInterval, func(interval *time.Duration) string { - if interval == nil { - return "" - } - return interval.String() + return durationToString(interval) }).Untyped().(pulumi.StringOutput),Also applies to: 143-149
deployments/pulumi/pkg/component.go (2)
44-51: Consider using a pointer receiver forComponentArgsto maintain consistency.The
NewComponentfunction takesargs ComponentArgsby value but then calls theSetDefaultsmethod on it. This modifies the local copy only, which might be confusing since other components use pointer receivers. Even though it doesn't affect functionality, it's better for consistency.-func NewComponent(ctx *pulumi.Context, name string, args ComponentArgs, opts ...pulumi.ResourceOption) (*Component, error) { +func NewComponent(ctx *pulumi.Context, name string, args *ComponentArgs, opts ...pulumi.ResourceOption) (*Component, error) { cmp := &Component{} err := ctx.RegisterComponentResource("Formance:Ledger", name, cmp, opts...) if err != nil { return nil, err } args.SetDefaults()
68-87: Improve resource dependency clarity with named variables.The code adds dependencies using array literals, which makes it hard to understand why specific dependencies are required.
-options = append(options, pulumi.DependsOn([]pulumi.Resource{ - cmp.Namespace, -})) +// Define dependencies with meaningful variable names +namespaceDependency := []pulumi.Resource{cmp.Namespace} +options = append(options, pulumi.DependsOn(namespaceDependency)) // Later in the code -options = append(options, pulumi.DependsOn([]pulumi.Resource{ - // don't depend on storage since it includes migrations - // we just need the database to be up, migrations will be run in background - // we also need to have credentials ready for the API and Worker - cmp.Storage.DatabaseComponent, - cmp.Storage.Credentials, - cmp.Storage.Service, -})) +// Define database dependencies with clear intent +databaseDependencies := []pulumi.Resource{ + // don't depend on storage since it includes migrations + // we just need the database to be up, migrations will be run in background + // we also need to have credentials ready for the API and Worker + cmp.Storage.DatabaseComponent, + cmp.Storage.Credentials, + cmp.Storage.Service, +} +options = append(options, pulumi.DependsOn(databaseDependencies))deployments/pulumi/pkg/worker/component.go (1)
37-101: Break downNewComponentfunction for better maintainability.The
NewComponentfunction handles component registration, environment variable setup, and deployment creation in a single block. This makes it harder to maintain and test.Consider refactoring the function to break it into smaller, more focused functions:
+func createDeployment(ctx *pulumi.Context, parent pulumi.Resource, args ComponentArgs, envVars corev1.EnvVarArray) (*appsv1.Deployment, error) { + return appsv1.NewDeployment(ctx, "ledger-worker", &appsv1.DeploymentArgs{ + // ... existing deployment args + }, pulumi.Parent(parent)) +} +func buildEnvironmentVariables(ctx *pulumi.Context, args ComponentArgs) (corev1.EnvVarArray, error) { + if args.Database == nil { + return nil, fmt.Errorf("database component is required but was not provided") + } + + envVars := corev1.EnvVarArray{} + envVars = append(envVars, corev1.EnvVarArgs{ + Name: pulumi.String("DEBUG"), + Value: utils.BoolToString(args.Debug).Untyped().(pulumi.StringOutput), + }) + + envVars = append(envVars, args.Database.GetEnvVars()...) + if otel := args.Otel; otel != nil { + envVars = append(envVars, args.Otel.GetEnvVars(ctx.Context())...) + } + + return envVars, nil +} func NewComponent(ctx *pulumi.Context, name string, args ComponentArgs, opts ...pulumi.ResourceOption) (*Component, error) { cmp := &Component{} err := ctx.RegisterComponentResource("Formance:Ledger:Worker", name, cmp, opts...) if err != nil { return nil, err } - envVars := corev1.EnvVarArray{} - envVars = append(envVars, corev1.EnvVarArgs{ - Name: pulumi.String("DEBUG"), - Value: utils.BoolToString(args.Debug).Untyped().(pulumi.StringOutput), - }) - - envVars = append(envVars, args.Database.GetEnvVars()...) - if otel := args.Otel; otel != nil { - envVars = append(envVars, args.Otel.GetEnvVars(ctx.Context())...) - } + envVars, err := buildEnvironmentVariables(ctx, args) + if err != nil { + return nil, err + } - cmp.Deployment, err = appsv1.NewDeployment(ctx, "ledger-worker", &appsv1.DeploymentArgs{ - // ... deployment args - }, pulumi.Parent(cmp)) + cmp.Deployment, err = createDeployment(ctx, cmp, args, envVars) if err != nil { return nil, fmt.Errorf("creating deployment: %w", err) } // ... rest of the function remains unchangeddeployments/pulumi/pkg/api/deployment.go (2)
57-61: createDeploymentArgs struct is clear.
CombiningCommonArgsandArgsplus theDatabasereference is coherent. Just be mindful of potential naming collisions if you embed more fields later.
63-124: Consider refactoring repeated environment variable logic.
Multiple code blocks convert nonzero values to strings. To reduce duplication, create a small helper function (e.g.,toStringIfNotZero(size int) string) and apply it in these environment variables.deployments/pulumi/pkg/storage/component.go (3)
6-6: Avoid using dot imports
It's generally considered a best practice to avoid dot imports, as they can lead to namespace pollution and reduce code clarity.Apply this diff to remove the dot import:
-import ( - "errors" - "fmt" - . "github.com/formancehq/ledger/deployments/pulumi/pkg/utils" - ... +import ( + "errors" + "fmt" + "github.com/formancehq/ledger/deployments/pulumi/pkg/utils" + ...
93-93: Consider making the database name configurable
Currently, the URI is hard-coded to/postgres. If you intend to support other database names, expose a configuration option or retrieve it from theDatabaseComponent.
203-205: Address the TODO to avoid blocking
Making the creation or initialization of the service asynchronous can help improve deployment speed. Consider opening a new issue or implementing a background job approach here.Do you want me to propose a solution or open a new issue to track this async enhancement?
deployments/pulumi/docs/schema.md (2)
3-3: Add a top-level description
A top-level description in the root of the JSON document helps clarify the schema’s purpose and usage for readers and automated tools.
5-5: Fix heading levels for better Markdown compliance
Markdown guidelines recommend incrementing heading levels only one at a time. Multiple headings here jump levels (like h2 → h4), which can cause confusion and hamper readability.Below is an example snippet converting h4 headings to h3 to maintain proper hierarchy:
-#### Type: `object` +### Type: `object`Also applies to: 16-16, 36-36, 57-57, 72-72, 85-85, 100-100, 117-117, 131-131, 146-146, 160-160, 174-174, 187-187, 200-200, 219-219, 232-232
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
5-5: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
deployments/pulumi/pkg/config/config.go (1)
399-408: Consider returning an error
If theStorageorAPIconfiguration is invalid, returning an error instead of constructing a partially validComponentArgscan help prevent unexpected runtime failures. You may modifyToInput()to return(pulumi_ledger.ComponentArgs, error).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
deployments/pulumi/docs/schema.jsonis excluded by!**/*.jsondeployments/pulumi/go.modis excluded by!**/*.moddeployments/pulumi/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (16)
deployments/pulumi/README.md(1 hunks)deployments/pulumi/docs/schema.md(1 hunks)deployments/pulumi/justfile(1 hunks)deployments/pulumi/pkg/api/deployment.go(9 hunks)deployments/pulumi/pkg/component.go(1 hunks)deployments/pulumi/pkg/config/config.go(1 hunks)deployments/pulumi/pkg/devbox/component.go(1 hunks)deployments/pulumi/pkg/storage/component.go(1 hunks)deployments/pulumi/pkg/storage/component_postgres.go(1 hunks)deployments/pulumi/pkg/storage/setup_postgres.go(1 hunks)deployments/pulumi/pkg/storage/setup_rds.go(1 hunks)deployments/pulumi/pkg/utils/args_common.go(1 hunks)deployments/pulumi/pkg/utils/args_otel.go(1 hunks)deployments/pulumi/pkg/worker/component.go(3 hunks)deployments/pulumi/tools/main.go(1 hunks)internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- deployments/pulumi/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- deployments/pulumi/pkg/storage/setup_postgres.go
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
deployments/pulumi/docs/schema.md
5-5: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
16-16: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
36-36: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
57-57: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
72-72: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
85-85: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
100-100: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
117-117: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
131-131: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
146-146: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
160-160: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
174-174: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
187-187: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
200-200: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
219-219: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
232-232: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (19)
deployments/pulumi/justfile (1)
1-5: LGTM: Well-structured documentation update script.The justfile recipe is correctly implemented with a proper shebang and a clear purpose - automating the generation of schema documentation from the codebase.
deployments/pulumi/tools/main.go (1)
33-35: LGTM: Proper error handling for AddGoComments.Good implementation of error handling for the AddGoComments function.
deployments/pulumi/pkg/utils/args_common.go (1)
8-14: LGTM: Well-structured CommonArgs type.The
CommonArgsstruct is well-defined with appropriate input types for Pulumi deployment configuration.internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql (1)
11-17: LGTM: Well-implemented trigger creation.The trigger creation logic is well-structured and includes helpful comments explaining the Azure Postgres limitations that necessitate this approach.
deployments/pulumi/pkg/api/deployment.go (6)
1-1: Renaming package looks good.
No issues spotted with moving frompulumi_ledgertoapi.
5-6: Imported packages seem correct.
All added imports (storage,utils, andtime) are sensible and align with the usage below.Also applies to: 12-12
15-25: Switching to a duration-based GracePeriod is appropriate.
Usingpulumix.Input[time.Duration]clarifies time-based intent. Ensure downstream consumption correctly parses zero or negative durations.
27-55: Defaulting fields.
All defaulting logic is concise and consistent. Check if it’s valid to accept zero values (especially for Parallels, BulkMaxSize, etc.) or if you want to reject negative/invalid inputs.
125-129: Extending envVars with Database and Otel is straightforward.
The integration withDatabase.GetEnvVars()andOtel.GetEnvVars(...)reads cleanly.
130-201: Ledger deployment creation looks fine.
The deployment resource, labels, image, and probes align with best practices. No concerns spotted.deployments/pulumi/pkg/storage/setup_rds.go (3)
10-19: Struct includes default credentials.
DefiningMasterUsernameandMasterPasswordwith weak defaults (e.g., “root”, “password”) is a known security risk. Consider requiring strong credentials or externalizing them for production.
21-74: Hard-coded fallback to "root"/"password".
Reusing weak defaults inSetDefaults()can lead to inadvertent usage in production. Prompt users to override them or validate stronger values.
104-143: Setup logic for RDS cluster usage vs creation is sound.
Properly returns errors if neither or both cluster configurations are provided. Implementation appears correct.deployments/pulumi/pkg/storage/component_postgres.go (1)
63-64: Defaulting to "root"/"password" again poses a security risk.
Avoid embedding weak credentials in source. Use environment variables or generated secrets for production.deployments/pulumi/pkg/config/config.go (5)
62-71: Add nil checks for RDSThe
RDSDatabasestruct may trigger a nil pointer dereference ifa.CreateClusterora.UseClusteris nil when calling.toInput(). To fix it:func (a *RDSDatabase) toInput() *storage.RDSDatabaseArgs { if a == nil { return nil } + var createCluster *storage.RDSDatabaseCreateArgs + if a.CreateCluster != nil { + createCluster = a.CreateCluster.toInput() + } + + var useCluster *storage.RDSUseExistingClusterArgs + if a.UseCluster != nil { + useCluster = a.UseCluster.toInput() + } + return &storage.RDSDatabaseArgs{ - CreateCluster: a.CreateCluster.toInput(), - UseCluster: a.UseCluster.toInput(), + CreateCluster: createCluster, + UseCluster: useCluster, } }
130-132: Avoid panicking for missing configurationPanicking can disrupt Pulumi previews and partial deployments. Instead, return an error to gracefully stop the deployment:
-if !a.Install { - panic("uri must be provided if install is false") -} +if !a.Install { + return nil, fmt.Errorf("uri must be provided if `install` is false") +}
290-304: Perform nil checks for Jaeger and OTLPAccessing
t.Jaeger.Endpointandt.OTLP.Modemay cause a panic ifJaegerorOTLPis nil. Consider this safer approach:func (t *OtelTraces) toInput() *utils.OtelTracesArgs { if t == nil { return nil } + args := &utils.OtelTracesArgs{ + OtelTracesBatch: pulumix.Val(t.Batch), + OtelTracesExporterFlag: pulumix.Val(t.ExporterFlag), + } + + if t.Jaeger != nil { + args.OtelTracesExporterJaegerEndpoint = pulumix.Val(t.Jaeger.Endpoint) + args.OtelTracesExporterJaegerUser = pulumix.Val(t.Jaeger.User) + args.OtelTracesExporterJaegerPassword = pulumix.Val(t.Jaeger.Password) + } + + if t.OTLP != nil { + args.OtelTracesExporterOTLPMode = pulumix.Val(t.OTLP.Mode) + args.OtelTracesExporterOTLPEndpoint = pulumix.Val(t.OTLP.Endpoint) + args.OtelTracesExporterOTLPInsecure = pulumix.Val(t.OTLP.Insecure) + } + + return args - return &utils.OtelTracesArgs{ - OtelTracesBatch: pulumix.Val(t.Batch), - OtelTracesExporterFlag: pulumix.Val(t.ExporterFlag), - OtelTracesExporterJaegerEndpoint: pulumix.Val(t.Jaeger.Endpoint), - OtelTracesExporterJaegerUser: pulumix.Val(t.Jaeger.User), - OtelTracesExporterJaegerPassword: pulumix.Val(t.Jaeger.Password), - OtelTracesExporterOTLPMode: pulumix.Val(t.OTLP.Mode), - OtelTracesExporterOTLPEndpoint: pulumix.Val(t.OTLP.Endpoint), - OtelTracesExporterOTLPInsecure: pulumix.Val(t.OTLP.Insecure), - } }
337-351: Check if OtelMetricsOTLP is nilAccessing
m.OtelMetricsOTLP.Modeor other fields may cause a panic ifOtelMetricsOTLPis nil. Use a safer approach:func (m *OtelMetrics) toInput() *utils.OtelMetricsArgs { if m == nil { return nil } - return &utils.OtelMetricsArgs{ - OtelMetricsExporterPushInterval: pulumix.Val(m.PushInterval), - OtelMetricsRuntime: pulumix.Val(m.Runtime), - OtelMetricsRuntimeMinimumReadMemStatsInterval: pulumix.Val(m.RuntimeMinimumReadMemStatsInterval), - OtelMetricsExporter: pulumix.Val(m.Exporter), - OtelMetricsKeepInMemory: pulumix.Val(m.KeepInMemory), - OtelMetricsExporterOTLPMode: pulumix.Val(m.OtelMetricsOTLP.Mode), - OtelMetricsExporterOTLPEndpoint: pulumix.Val(m.OtelMetricsOTLP.Endpoint), - OtelMetricsExporterOTLPInsecure: pulumix.Val(m.OtelMetricsOTLP.Insecure), - } + args := &utils.OtelMetricsArgs{ + OtelMetricsExporterPushInterval: pulumix.Val(m.PushInterval), + OtelMetricsRuntime: pulumix.Val(m.Runtime), + OtelMetricsRuntimeMinimumReadMemStatsInterval: pulumix.Val(m.RuntimeMinimumReadMemStatsInterval), + OtelMetricsExporter: pulumix.Val(m.Exporter), + OtelMetricsKeepInMemory: pulumix.Val(m.KeepInMemory), + } + if m.OtelMetricsOTLP != nil { + args.OtelMetricsExporterOTLPMode = pulumix.Val(m.OtelMetricsOTLP.Mode) + args.OtelMetricsExporterOTLPEndpoint = pulumix.Val(m.OtelMetricsOTLP.Endpoint) + args.OtelMetricsExporterOTLPInsecure = pulumix.Val(m.OtelMetricsOTLP.Insecure) + } + return args }
363-365: Validate allowed values for ImagePullPolicy
Kubernetes only supports "Always", "IfNotPresent", or "Never" forimagePullPolicy. Consider adding logic to verify thatc.ImagePullPolicymatches one of these values and return an error if not.
| do $$ | ||
| declare | ||
| _ledger record; | ||
| _vsql text; | ||
| begin | ||
| -- cannot disable triggers at session level on Azure Postgres with no superuser privileges. | ||
| -- so we modify the trigger acting on transaction update to be triggered only if the metadata column is updated. | ||
| -- by the way, it's a good move to not trigger the update_transaction_metadata_history function on every update if not necessary. | ||
| for _ledger in select * from _system.ledgers where bucket = current_schema loop | ||
| _vsql = 'create or replace trigger "update_transaction_metadata_history_' || _ledger.id || '" after update of metadata on "transactions" for each row when (new.ledger = ''' || _ledger.name || ''') execute procedure update_transaction_metadata_history()'; | ||
| execute _vsql; | ||
| end loop; | ||
| end | ||
| $$; No newline at end of file |
There was a problem hiding this comment.
Remove duplicate trigger creation code block.
There's a duplicate block of code that creates the same triggers as the code on lines 11-17. This redundancy should be removed as it's performing the same operation twice, which can lead to confusion and potential issues during migration.
-do $$
- declare
- _ledger record;
- _vsql text;
- begin
- -- cannot disable triggers at session level on Azure Postgres with no superuser privileges.
- -- so we modify the trigger acting on transaction update to be triggered only if the metadata column is updated.
- -- by the way, it's a good move to not trigger the update_transaction_metadata_history function on every update if not necessary.
- for _ledger in select * from _system.ledgers where bucket = current_schema loop
- _vsql = 'create or replace trigger "update_transaction_metadata_history_' || _ledger.id || '" after update of metadata on "transactions" for each row when (new.ledger = ''' || _ledger.name || ''') execute procedure update_transaction_metadata_history()';
- execute _vsql;
- end loop;
- end
-$$;📝 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.
| do $$ | |
| declare | |
| _ledger record; | |
| _vsql text; | |
| begin | |
| -- cannot disable triggers at session level on Azure Postgres with no superuser privileges. | |
| -- so we modify the trigger acting on transaction update to be triggered only if the metadata column is updated. | |
| -- by the way, it's a good move to not trigger the update_transaction_metadata_history function on every update if not necessary. | |
| for _ledger in select * from _system.ledgers where bucket = current_schema loop | |
| _vsql = 'create or replace trigger "update_transaction_metadata_history_' || _ledger.id || '" after update of metadata on "transactions" for each row when (new.ledger = ''' || _ledger.name || ''') execute procedure update_transaction_metadata_history()'; | |
| execute _vsql; | |
| end loop; | |
| end | |
| $$; |
| installDevBox, err := internals.UnsafeAwaitOutput(ctx.Context(), args.InstallDevBox.ToOutput(ctx.Context())) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if installDevBox.Value != nil && installDevBox.Value.(bool) { | ||
| cmp.Devbox, err = devbox.NewComponent(ctx, "devbox", devbox.ComponentArgs{ | ||
| CommonArgs: args.CommonArgs, | ||
| Storage: cmp.Storage, | ||
| }, options...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for UnsafeAwaitOutput and type assertion.
The code calls UnsafeAwaitOutput and performs a type assertion on its result without proper error checks. This could lead to panics if the type assertion fails.
installDevBox, err := internals.UnsafeAwaitOutput(ctx.Context(), args.InstallDevBox.ToOutput(ctx.Context()))
if err != nil {
return nil, err
}
-if installDevBox.Value != nil && installDevBox.Value.(bool) {
+if installDevBox.Value != nil {
+ boolValue, ok := installDevBox.Value.(bool)
+ if !ok {
+ return nil, fmt.Errorf("unexpected type for InstallDevBox: %T, expected bool", installDevBox.Value)
+ }
+ if boolValue {
cmp.Devbox, err = devbox.NewComponent(ctx, "devbox", devbox.ComponentArgs{
CommonArgs: args.CommonArgs,
Storage: cmp.Storage,
}, options...)
if err != nil {
return nil, err
}
}
+}📝 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.
| installDevBox, err := internals.UnsafeAwaitOutput(ctx.Context(), args.InstallDevBox.ToOutput(ctx.Context())) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if installDevBox.Value != nil && installDevBox.Value.(bool) { | |
| cmp.Devbox, err = devbox.NewComponent(ctx, "devbox", devbox.ComponentArgs{ | |
| CommonArgs: args.CommonArgs, | |
| Storage: cmp.Storage, | |
| }, options...) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| installDevBox, err := internals.UnsafeAwaitOutput(ctx.Context(), args.InstallDevBox.ToOutput(ctx.Context())) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if installDevBox.Value != nil { | |
| boolValue, ok := installDevBox.Value.(bool) | |
| if !ok { | |
| return nil, fmt.Errorf("unexpected type for InstallDevBox: %T, expected bool", installDevBox.Value) | |
| } | |
| if boolValue { | |
| cmp.Devbox, err = devbox.NewComponent(ctx, "devbox", devbox.ComponentArgs{ | |
| CommonArgs: args.CommonArgs, | |
| Storage: cmp.Storage, | |
| }, options...) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| } |
| Name: pulumi.String("worker"), | ||
| Image: ledgerImage, | ||
| Image: utils.GetImage(args.Tag), | ||
| ImagePullPolicy: args.ImagePullPolicy.ToOutput(ctx.Context()).Untyped().(pulumi.StringOutput), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add validation for the image tag.
The function calls utils.GetImage(args.Tag) without validating that args.Tag has a valid value. Without proper validation, this could lead to using an invalid or insecure image.
🏁 Script executed:
#!/bin/bash
# Check if utils.GetImage function has internal validation
# Look for the GetImage implementation
rg -p "func GetImage" -g "*.go" --context=10Length of output: 549
Action Required: Add Validation for the Image Tag
In deployments/pulumi/pkg/worker/component.go (lines 79–81), the call to utils.GetImage(args.Tag) passes the tag without any validation. The implementation in deployments/pulumi/pkg/utils/convert.go confirms that no internal checks are performed on the tag value, potentially leading to the use of an invalid or insecure image. Please add explicit validation for args.Tag (e.g., checking for non-empty and matching an expected regex) before invoking utils.GetImage.
1d57b13 to
0c13bcf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
deployments/pulumi/docs/schema.md (1)
3-247: 🛠️ Refactor suggestionAdd Missing Model Descriptions
Many of the schema sections (e.g., API, Config, ConnectivityDatabase, Ingress, Monitoring, OtelMetrics, OtelMetricsOTLP, OtelTraces, OtelTracesJaeger, OtelTracesOTLP, PostgresDatabase, RDSClusterCreate, RDSDatabase, RDSUseExistingCluster, Storage) display "No description provided for this model." Enhancing these sections with descriptive text via thedescriptionkey can greatly improve the documentation's clarity and usefulness for users.For example, consider adding:
"description": "A brief description of the model's purpose and usage."to each section.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
5-5: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
16-16: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
36-36: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
57-57: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
72-72: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
85-85: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
100-100: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
117-117: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
131-131: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
146-146: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
160-160: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
174-174: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
187-187: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
206-206: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
219-219: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
232-232: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
🧹 Nitpick comments (27)
deployments/pulumi/main_test.go (1)
32-39: Marshaling values to JSON strings may cause complex debugging.
Serializing arbitrary data to JSON and storing it as a string can complicate debugging. Consider storing certain fields (e.g. booleans or numbers) as typed values if Pulumi supports them directly.deployments/pulumi/main.go (1)
30-31: Single returnerris consistent with the streamlined approach.
If there are multiple operations, consider returning a descriptive wrapped error to help debug deployment failures quickly.deployments/pulumi/pkg/api/component.go (1)
64-69: Registering outputs to Pulumi fosters clarity.
Returning an empty map is valid; consider exporting references to created resources if widely needed by other stacks.deployments/pulumi/pkg/api/ingress.go (1)
12-16: Consider adding validation or docstrings for fields inIngressArgs.
Without input validation or docstrings, users might encounter unexpected behavior if these fields are empty or invalid.deployments/pulumi/pkg/component.go (2)
27-32: Consider defaults forTimeoutandInstallDevBox.
WhileSetDefaults()updatesDatabase,CommonArgs,API, andWorker, it doesn't set defaults for these fields.
122-124: Onlydeployment-nameis registered as a resource output.
If there's a need to reference the namespace or other resources, consider adding them here.deployments/pulumi/pkg/storage/component_rds.go (1)
1-21: Add docstrings or references toRDSComponentArgsandRDSDatabaseComponent.
This helps clarify expected usage for cluster creation arguments.deployments/pulumi/pkg/storage/setup_rds.go (1)
104-143: Consider avoiding panic in production code.The
panic("unreachable")can be replaced with a descriptive error return to aid debugging. Panics can halt your entire deployment if ever triggered by a logic flaw or future changes.deployments/pulumi/pkg/storage/component.go (2)
71-157: Environment variable naming consistency.All settings use
POSTGRES_-prefixed environment variables, even when using Aurora or another RDS engine. If you plan to support additional engines in the future, consider using more general naming likeDB_USERNAME,DB_PASSWORD, etc.
159-250: Overall component creation flow is well-structured.The logic for choosing between Postgres or RDS, creating a secret, and optionally setting up a service for external DB usage is clearly organized. However, consider splitting some of the migration job code into smaller functions for improved readability.
deployments/pulumi/docs/schema.md (17)
1-4: Add Root JSON Schema Description
The document begins with a note stating that the JSON Schema is missing a description. To improve clarity and context, consider adding a top-leveldescriptionkey in the root JSON document.
5-8: Fix Heading Level for "Type" Section at the Top
The heading on line 5 uses###("Type:object(?)"), which skips a level relative to the document’s top-level heading. For better structure and to satisfy markdownlint rules (MD001), consider using##instead.Example diff:
-### Type: `object(?)` +## Type: `object(?)`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
5-5: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
12-19: Correct Heading Level in the API Section
Within the API section, the model’s type is defined with a####heading on line 16. To maintain a proper incremental heading structure (following an## APIheading), this should be demoted by one level (to###).Example diff:
-#### Type: `object` +### Type: `object`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
16-16: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
32-39: Update Heading Level in the Config Section
Similarly, in the Config section starting on line 32, the type is introduced with a####header on line 36. This should be adjusted to###so that it properly nests under the## Configheading.Example diff:
-#### Type: `object` +### Type: `object`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
36-36: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
53-60: Align Heading Level in the ConnectivityDatabase Section
The ConnectivityDatabase section (line 53 onward) uses a####heading on line 57. To remain consistent with the parent## ConnectivityDatabaseheading, please change it to###.Example diff:
-#### Type: `object` +### Type: `object`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
57-57: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
68-75: Fix Heading Level in the Ingress Section
The Ingress section includes a####header on line 72. Adjust it to###to keep proper heading progression after the## Ingressheading.Example diff:
-#### Type: `object` +### Type: `object`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
72-72: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
81-88: Correct Heading Level in the Monitoring Section
Within the Monitoring section, the type header on line 86 is at the####level. It should be changed to###to align with the## Monitoringheading.Example diff:
-#### Type: `object` +### Type: `object`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
85-85: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
96-103: Revise Heading Level in the OtelMetrics Section
In the OtelMetrics section, the type is declared with a####header on line 100. Please adjust this to###to conform with the document structure.Example diff:
-#### Type: `object` +### Type: `object`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
100-100: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
113-120: Harmonize Heading Level in the OtelMetricsOTLP Section
The OtelMetricsOTLP section (line 113 onward) uses a####heading on line 117. For consistency, change it to###.Example diff:
-#### Type: `object` +### Type: `object`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
117-117: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
127-134: Standardize Heading Level in the OtelTraces Section
The OtelTraces section features a type header defined with####on line 131. It is recommended to downgrade this to###for proper nesting under## OtelTraces.Example diff:
-#### Type: `object` +### Type: `object`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
131-131: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
142-149: Update Heading Level in the OtelTracesJaeger Section
In the OtelTracesJaeger section, the type heading on line 146 is at level 4 (####). Change it to level 3 (###) to maintain a coherent hierarchy.Example diff:
-#### Type: `object` +### Type: `object`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
146-146: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
156-163: Adjust Heading Level in the OtelTracesOTLP Section
The type declaration in the OtelTracesOTLP section on line 160 uses####. It should be updated to###to conform with the overall heading structure.Example diff:
-#### Type: `object` +### Type: `object`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
160-160: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
170-177: Refine Heading Level in the PostgresDatabase Section
For the PostgresDatabase section, the heading on line 174 is#### Type: 'object'and would be more appropriately set to###for consistency.Example diff:
-#### Type: `object` +### Type: `object`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
174-174: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
183-190: Correct Heading Level in the RDSClusterCreate Section
Within the RDSClusterCreate section, the type header (line 187) should be adjusted from####to###to align correctly with the parent heading.Example diff:
-#### Type: `object` +### Type: `object`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
187-187: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
202-209: Standardize Heading Level in the RDSDatabase Section
The header on line 206 (#### Type: 'object') in the RDSDatabase section should be changed to###to maintain uniform heading progression.Example diff:
-#### Type: `object` +### Type: `object`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
206-206: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
215-222: Fix Heading Level in the RDSUseExistingCluster Section
In the RDSUseExistingCluster section, update the heading on line 219 from####to###for proper hierarchy under## RDSUseExistingCluster.Example diff:
-#### Type: `object` +### Type: `object`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
219-219: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
228-235: Update Heading Level in the Storage Section
At line 232 in the Storage section, the heading is#### Type: 'object'. Adjust this to###to match the expected heading sequence.Example diff:
-#### Type: `object` +### Type: `object`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
232-232: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
deployments/pulumi/docs/schema.jsonis excluded by!**/*.jsondeployments/pulumi/go.modis excluded by!**/*.moddeployments/pulumi/go.sumis excluded by!**/*.sum,!**/*.sumtools/k8s-stack/Pulumi.yamlis excluded by!**/*.yamltools/k8s-stack/go.modis excluded by!**/*.modtools/k8s-stack/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (31)
deployments/pulumi/README.md(1 hunks)deployments/pulumi/docs/schema.md(1 hunks)deployments/pulumi/justfile(1 hunks)deployments/pulumi/main.go(1 hunks)deployments/pulumi/main_test.go(2 hunks)deployments/pulumi/pkg/api/component.go(1 hunks)deployments/pulumi/pkg/api/deployment.go(9 hunks)deployments/pulumi/pkg/api/ingress.go(3 hunks)deployments/pulumi/pkg/api/service.go(2 hunks)deployments/pulumi/pkg/component.go(1 hunks)deployments/pulumi/pkg/component_args.go(0 hunks)deployments/pulumi/pkg/config/config.go(1 hunks)deployments/pulumi/pkg/devbox/component.go(1 hunks)deployments/pulumi/pkg/image.go(0 hunks)deployments/pulumi/pkg/storage/component.go(1 hunks)deployments/pulumi/pkg/storage/component_external.go(1 hunks)deployments/pulumi/pkg/storage/component_postgres.go(1 hunks)deployments/pulumi/pkg/storage/component_rds.go(1 hunks)deployments/pulumi/pkg/storage/migrate.go(2 hunks)deployments/pulumi/pkg/storage/setup_postgres.go(1 hunks)deployments/pulumi/pkg/storage/setup_rds.go(1 hunks)deployments/pulumi/pkg/utils/args_common.go(1 hunks)deployments/pulumi/pkg/utils/args_otel.go(1 hunks)deployments/pulumi/pkg/utils/convert.go(1 hunks)deployments/pulumi/pkg/worker/component.go(3 hunks)deployments/pulumi/tools/main.go(1 hunks)internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql(1 hunks)tools/k8s-stack/justfile(0 hunks)tools/k8s-stack/main.go(0 hunks)tools/k8s-stack/pkg/postgres.go(0 hunks)tools/k8s-stack/pkg/stack.go(0 hunks)
💤 Files with no reviewable changes (6)
- tools/k8s-stack/justfile
- deployments/pulumi/pkg/image.go
- tools/k8s-stack/main.go
- tools/k8s-stack/pkg/stack.go
- deployments/pulumi/pkg/component_args.go
- tools/k8s-stack/pkg/postgres.go
🚧 Files skipped from review as they are similar to previous changes (12)
- deployments/pulumi/justfile
- deployments/pulumi/tools/main.go
- deployments/pulumi/README.md
- deployments/pulumi/pkg/utils/convert.go
- internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql
- deployments/pulumi/pkg/api/deployment.go
- deployments/pulumi/pkg/storage/setup_postgres.go
- deployments/pulumi/pkg/devbox/component.go
- deployments/pulumi/pkg/api/service.go
- deployments/pulumi/pkg/config/config.go
- deployments/pulumi/pkg/storage/component_external.go
- deployments/pulumi/pkg/utils/args_common.go
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
deployments/pulumi/docs/schema.md
5-5: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
16-16: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
36-36: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
57-57: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
72-72: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
85-85: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
100-100: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
117-117: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
131-131: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
146-146: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
160-160: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
174-174: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
187-187: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
206-206: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
219-219: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
232-232: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (46)
deployments/pulumi/pkg/worker/component.go (7)
24-29: Clarify the purpose of the unused Service field.The
Servicefield is declared in theComponentstruct but never assigned a value in theNewComponentfunction.Either implement the Service creation logic or remove this unused field from the struct.
31-35: Add validation for the Database dependency.The
Databasefield is used on line 50 without any nil checks. This could lead to a panic if the database component is not provided.Add validation at the beginning of the
NewComponentfunction:func NewComponent(ctx *pulumi.Context, name string, args ComponentArgs, opts ...pulumi.ResourceOption) (*Component, error) { cmp := &Component{} err := ctx.RegisterComponentResource("Formance:Ledger:Worker", name, cmp, opts...) if err != nil { return nil, err } + if args.Database == nil { + return nil, fmt.Errorf("database component is required") + }
47-48: LGTM - Good abstraction for boolean conversion.Using the utility function
utils.BoolToStringimproves code consistency and readability.
79-81: Validate the image tag before usage.The function calls
utils.GetImage(args.Tag)without validating thatargs.Taghas a valid value.#!/bin/bash # Check if utils.GetImage has internal validation rg -p "func GetImage" -g "*.go" --context 5
92-98: Well-implemented error handling.The added error handling is comprehensive, with clear error messages that include the context of the failure.
37-101: Good overall component implementation structure.The
NewComponentfunction follows a consistent pattern with other components in the codebase:
- Creates the component object
- Registers it with Pulumi
- Creates the child resources
- Registers outputs
- Returns the component
This structure makes the code maintainable and follows Pulumi best practices.
76-76:Details
❓ Verification inconclusive
Verify type cast safety.
The cast from
args.TerminationGracePeriodSeconds.ToOutput(ctx.Context()).Untyped()topulumi.IntPtrOutputassumes the type will match correctly. Consider adding explicit validation or handling for potential type conversion errors.
🏁 Script executed:
#!/bin/bash # Check how TerminationGracePeriodSeconds is used in other places rg -A 3 "TerminationGracePeriodSeconds" -g "*.go" --no-filenameLength of output: 1906
Ensure Cast Safety for TerminationGracePeriodSeconds
The conversion from
args.TerminationGracePeriodSeconds.ToOutput(ctx.Context()).Untyped().(pulumi.IntPtrOutput)is employed consistently in the codebase, and its usage matches similar patterns elsewhere. That said, it relies on the assumption that the untyped output always conforms to
pulumi.IntPtrOutput. If there’s any chance that future changes or unexpected input could break this assumption, you might consider adding either an explicit validation step or a safe type assertion to catch conversion errors at runtime.
- Confirm that the API guarantees the output of
ToOutput(ctx.Context()).Untyped()will always be apulumi.IntPtrOutput.- If there’s any risk of mismatch, consider a safeguard (e.g., using the comma-ok idiom) or additional logging to handle potential type assertion failures.
deployments/pulumi/pkg/storage/migrate.go (7)
1-1: Package name refined to better represent its purpose.The package name change from
pulumi_ledgertostoragebetter represents the specific role of this package in managing storage components rather than the more generic previous name.
4-4: Good import path update for better code organization.The import path has been updated to use the modular
utilspackage, which promotes better separation of concerns and makes the codebase more maintainable.
11-14: Well-structured arguments encapsulation.The new
migrationArgsstruct effectively encapsulates related parameters by embeddingutils.CommonArgsand including a component pointer. This improves code readability and makes future extensions easier by centralizing parameter definitions.
16-16: Function signature improved for better semantics and flexibility.The function name change from
newMigrationJobtorunMigrateJobbetter reflects its purpose. The new signature with a single structured argument and variadic options parameters improves maintainability and flexibility.
17-23: Enhanced environment variable management.The new approach to environment variable management is more systematic and flexible. Creating an
EnvVarArrayand appending component environment variables allows for better organization and extensibility.
39-39: Centralized image retrieval through utility function.Using
utils.GetImage(args.Tag)instead of a direct call centralizes the image retrieval logic, promoting code reuse and making future changes to image handling easier to implement.
47-47:Details
✅ Verification successful
Important resource replacement strategy added.
Adding
pulumi.DeleteBeforeReplace(true)ensures that the resource is deleted before being replaced, which is crucial for resources that can't be updated in place. This guarantees clean deployments with no residual state from previous versions.
🏁 Script executed:
#!/bin/bash # Let's verify how DeleteBeforeReplace is used in similar deployment patterns throughout the codebase # Search for other instances of DeleteBeforeReplace to confirm consistent usage pattern echo "Checking for other instances of DeleteBeforeReplace in the codebase:" rg "DeleteBeforeReplace" --type goLength of output: 278
Validated Resource Replacement Strategy
The usage ofpulumi.DeleteBeforeReplace(true)at line 47 indeployments/pulumi/pkg/storage/migrate.gocorrectly implements the intended strategy by ensuring that the resource is fully deleted before being replaced. This is essential for resources that cannot be updated in-place, resulting in cleaner deployments without residual artifacts.deployments/pulumi/main_test.go (4)
4-4: No issues with the new import.
Importing"encoding/json"here is appropriate to handle the JSON marshaling of the newinterface{}-based configuration.
16-16: Map ofanycould reduce type safety.
Switching the config type tomap[string]anyadds flexibility but you lose compile-time checks. Verify that you handle all expected key-value types in tests to avoid runtime errors.
21-27: Configuration object looks good.
Defining nested objects forstorageandpostgreswith aninstallflag aligns well with Pulumi best practices for structured settings. Consider adding additional test cases to verify the effect of differentinstallvalues.
45-45: Config assignment is consistent.
Assigning the preprocessedconfigmap toConfigensures the Pulumi test harness can consume these settings without further changes.deployments/pulumi/main.go (5)
5-5: New config import aligns with the restructured deployment.
Pulling theconfigpackage explicitly is consistent with the new flexible configuration approach.
10-10: Inline pulumi.Run usage is clean and reduces function complexity.
Embedding the logic directly inpulumi.Runcenters the setup in a single closure, simplifying reading.
12-15: Ensure correct error handling from config.Load.
Check ifconfig.Load(ctx)can return partial success or if there's any scenario that needs an explicit fallback.
17-19: Instantiating the new component takes advantage of the config refactor.
ledger.NewComponentusage aligns with the new arguments-based approach. Make sure any removed code for resource creation is handled by the new component.
22-28: Avoid exporting secrets in plaintext .
Exportingcmp.Storage.DatabaseComponent.GetPassword()unencrypted can pose a security risk. Use Pulumi secrets (e.g.pulumi.ToSecret(...)) to avoid storing passwords unencrypted in state.deployments/pulumi/pkg/api/component.go (3)
26-31: Resource registration looks solid.
Registering the component resource up front ensures Pulumi tracks the component properly.
33-45: Deployment and service creation is adequately modularized.
SeparatingcreateDeploymentandcreateServiceis a good demonstration of the single-responsibility principle, increasing maintainability.
50-62: Conditional ingress resource creation is clear.
The optional logic for ingress is straightforward; ensure that thecreateIngressfunction includes proper error handling in deeper layers as well.deployments/pulumi/pkg/utils/args_otel.go (3)
1-3: Structured approach for OTEL configuration.
Introducing dedicated argument structs is a clean way to handle dynamic OTEL tracing and metrics settings.
43-47: Nil-safe check avoids runtime panic.
Earlyif args == nil { return envVars }ensures you skip environment variables gracefully when OTEL is disabled.
122-181: Comprehensive handling of metrics environment variables.
The metric-related environment variables handle optional durations, booleans, and strings effectively, ensuring robust configuration for multiple scenarios.deployments/pulumi/pkg/api/ingress.go (2)
18-21: Struct composition is a clean approach.
CombiningCommonArgswithIngressArgsensures DRY usage.
23-70: Verify casting withUntyped().(pulumi.StringOutput).
The type assertion might cause a runtime panic if there's a mismatch. Make sure the input values strictly resolve to the correct type.deployments/pulumi/pkg/component.go (6)
1-10: Imports and package rename seem consistent.
Renaming the package toledgerand importing the new modules align with the code restructuring.
17-25: Verify optional vs required fields inComponentArgs.
Currently, there's no validation of optional vs required fields. Consider adding checks or docstrings for clarity.
37-41: Nice modular approach.
Splitting separate resource references in theComponentstruct fosters maintainability and clarity.
44-46: Switching from pointer to value forargsis beneficial for immutability.
This approach reduces potential nil-pointer scenarios.
51-55: Good practice: callingSetDefaults()early.
This ensures consistent initialization across sub-components.
108-113: This code might panic ifInstallDevBoxis not a bool.
A type check is missing.deployments/pulumi/pkg/storage/component_rds.go (3)
23-41: Endpoints, credentials, and port getters look correct.
Returningpulumix.Inputensures consistent usage across Pulumi calls.
43-112: Validate mandatory fields innewRDSDatabaseComponent.
Currently, onlydbSubnetGroupNameis validated. Consider verifyingSnapshotIdentifier,MasterUsername, etc.
114-114: Implementation ofDatabaseComponentinterface is confirmed.
EnsuresRDSDatabaseComponentaligns with the expected contract.deployments/pulumi/pkg/storage/setup_rds.go (5)
10-19: Add validation for required fields in RDS cluster creation.While these fields are optional, you may want stricter validation for critical fields such as
UseSubnetGroupNameandEngineto prevent misconfiguration when provisioning real environments.
21-74: Hard-coded default credentials and engine version.Using
"root"/"password"as default credentials is insecure. Additionally, defaultingEngineVersionto"16"may not be supported in all AWS regions. Consider externalizing credentials and verifying that the default engine version is valid in the target region.- return "root" - return "password" + // Read from secure environment variables or a secrets store. + // Generate random strong credentials when not explicitly provided.
76-88: Defaults for existing cluster arguments look good.Setting them to empty strings by default is fine, and the usage in the
setupmethod already checks for non-empty values. No immediate issues here.
90-93: RDSDatabaseArgs structure appears consistent.The approach of grouping either a pre-existing cluster or creation parameters under the same struct is neat and readable.
95-102: Propagation of defaults is straightforward.Passing default values down to nested structs ensures consistency. This is good design for layered configuration objects.
deployments/pulumi/pkg/storage/component_postgres.go (1)
63-64: Hard-coded default credentials.Using
"root"/"password"can introduce security vulnerabilities. Recommend storing credentials in a secure secret manager, or generating them dynamically.- username := pulumix.Val("root") - password := pulumix.Val("password") + // Retrieve username and password from a secure configuration or random generator.
| if traces.OtelTracesBatch != nil { | ||
| envVars = append(envVars, corev1.EnvVarArgs{ | ||
| Name: pulumi.String("OTEL_TRACES_BATCH"), | ||
| Value: BoolToString(traces.OtelTracesBatch).Untyped().(pulumi.StringOutput), | ||
| }) | ||
| } | ||
| if traces.OtelTracesExporterFlag != nil { | ||
| envVars = append(envVars, corev1.EnvVarArgs{ | ||
| Name: pulumi.String("OTEL_TRACES_EXPORTER"), | ||
| Value: traces.OtelTracesExporterFlag.ToOutput(ctx).Untyped().(pulumi.StringOutput), | ||
| }) | ||
| } | ||
| if traces.OtelTracesExporterJaegerEndpoint != nil { | ||
| envVars = append(envVars, corev1.EnvVarArgs{ | ||
| Name: pulumi.String("OTEL_TRACES_EXPORTER_JAEGER_ENDPOINT"), | ||
| Value: traces.OtelTracesExporterJaegerEndpoint.ToOutput(ctx).Untyped().(pulumi.StringOutput), | ||
| }) | ||
| } | ||
| if traces.OtelTracesExporterJaegerUser != nil { | ||
| envVars = append(envVars, corev1.EnvVarArgs{ | ||
| Name: pulumi.String("OTEL_TRACES_EXPORTER_JAEGER_USER"), | ||
| Value: traces.OtelTracesExporterJaegerUser.ToOutput(ctx).Untyped().(pulumi.StringOutput), | ||
| }) | ||
| } | ||
| if traces.OtelTracesExporterJaegerPassword != nil { | ||
| envVars = append(envVars, corev1.EnvVarArgs{ | ||
| Name: pulumi.String("OTEL_TRACES_EXPORTER_JAEGER_PASSWORD"), | ||
| Value: traces.OtelTracesExporterJaegerPassword.ToOutput(ctx).Untyped().(pulumi.StringOutput), | ||
| }) | ||
| } | ||
| if traces.OtelTracesExporterOTLPMode != nil { | ||
| envVars = append(envVars, corev1.EnvVarArgs{ | ||
| Name: pulumi.String("OTEL_TRACES_EXPORTER_OTLP_MODE"), | ||
| Value: traces.OtelTracesExporterOTLPMode.ToOutput(ctx).Untyped().(pulumi.StringOutput), | ||
| }) | ||
| } | ||
| if traces.OtelTracesExporterOTLPEndpoint != nil { | ||
| envVars = append(envVars, corev1.EnvVarArgs{ | ||
| Name: pulumi.String("OTEL_TRACES_EXPORTER_OTLP_ENDPOINT"), | ||
| Value: traces.OtelTracesExporterOTLPEndpoint.ToOutput(ctx).Untyped().(pulumi.StringOutput), | ||
| }) | ||
| } | ||
| if traces.OtelTracesExporterOTLPInsecure != nil { | ||
| envVars = append(envVars, corev1.EnvVarArgs{ | ||
| Name: pulumi.String("OTEL_TRACES_EXPORTER_OTLP_INSECURE"), | ||
| Value: BoolToString(traces.OtelTracesExporterOTLPInsecure).Untyped().(pulumi.StringOutput), | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Exporting Jaeger credentials via environment variables might be insecure.
Check if you need to mark these variables as Pulumi secrets or consider more secure ways to inject credentials to avoid accidental leaks.
- Name: pulumi.String("OTEL_TRACES_EXPORTER_JAEGER_PASSWORD"),
- Value: traces.OtelTracesExporterJaegerPassword.ToOutput(ctx).Untyped().(pulumi.StringOutput),
+ Name: pulumi.String("OTEL_TRACES_EXPORTER_JAEGER_PASSWORD"),
+ Value: pulumi.ToSecret(traces.OtelTracesExporterJaegerPassword.ToOutput(ctx).Untyped().(pulumi.StringOutput)),📝 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.
| if traces.OtelTracesBatch != nil { | |
| envVars = append(envVars, corev1.EnvVarArgs{ | |
| Name: pulumi.String("OTEL_TRACES_BATCH"), | |
| Value: BoolToString(traces.OtelTracesBatch).Untyped().(pulumi.StringOutput), | |
| }) | |
| } | |
| if traces.OtelTracesExporterFlag != nil { | |
| envVars = append(envVars, corev1.EnvVarArgs{ | |
| Name: pulumi.String("OTEL_TRACES_EXPORTER"), | |
| Value: traces.OtelTracesExporterFlag.ToOutput(ctx).Untyped().(pulumi.StringOutput), | |
| }) | |
| } | |
| if traces.OtelTracesExporterJaegerEndpoint != nil { | |
| envVars = append(envVars, corev1.EnvVarArgs{ | |
| Name: pulumi.String("OTEL_TRACES_EXPORTER_JAEGER_ENDPOINT"), | |
| Value: traces.OtelTracesExporterJaegerEndpoint.ToOutput(ctx).Untyped().(pulumi.StringOutput), | |
| }) | |
| } | |
| if traces.OtelTracesExporterJaegerUser != nil { | |
| envVars = append(envVars, corev1.EnvVarArgs{ | |
| Name: pulumi.String("OTEL_TRACES_EXPORTER_JAEGER_USER"), | |
| Value: traces.OtelTracesExporterJaegerUser.ToOutput(ctx).Untyped().(pulumi.StringOutput), | |
| }) | |
| } | |
| if traces.OtelTracesExporterJaegerPassword != nil { | |
| envVars = append(envVars, corev1.EnvVarArgs{ | |
| Name: pulumi.String("OTEL_TRACES_EXPORTER_JAEGER_PASSWORD"), | |
| Value: traces.OtelTracesExporterJaegerPassword.ToOutput(ctx).Untyped().(pulumi.StringOutput), | |
| }) | |
| } | |
| if traces.OtelTracesExporterOTLPMode != nil { | |
| envVars = append(envVars, corev1.EnvVarArgs{ | |
| Name: pulumi.String("OTEL_TRACES_EXPORTER_OTLP_MODE"), | |
| Value: traces.OtelTracesExporterOTLPMode.ToOutput(ctx).Untyped().(pulumi.StringOutput), | |
| }) | |
| } | |
| if traces.OtelTracesExporterOTLPEndpoint != nil { | |
| envVars = append(envVars, corev1.EnvVarArgs{ | |
| Name: pulumi.String("OTEL_TRACES_EXPORTER_OTLP_ENDPOINT"), | |
| Value: traces.OtelTracesExporterOTLPEndpoint.ToOutput(ctx).Untyped().(pulumi.StringOutput), | |
| }) | |
| } | |
| if traces.OtelTracesExporterOTLPInsecure != nil { | |
| envVars = append(envVars, corev1.EnvVarArgs{ | |
| Name: pulumi.String("OTEL_TRACES_EXPORTER_OTLP_INSECURE"), | |
| Value: BoolToString(traces.OtelTracesExporterOTLPInsecure).Untyped().(pulumi.StringOutput), | |
| }) | |
| } | |
| } | |
| if traces.OtelTracesBatch != nil { | |
| envVars = append(envVars, corev1.EnvVarArgs{ | |
| Name: pulumi.String("OTEL_TRACES_BATCH"), | |
| Value: BoolToString(traces.OtelTracesBatch).Untyped().(pulumi.StringOutput), | |
| }) | |
| } | |
| if traces.OtelTracesExporterFlag != nil { | |
| envVars = append(envVars, corev1.EnvVarArgs{ | |
| Name: pulumi.String("OTEL_TRACES_EXPORTER"), | |
| Value: traces.OtelTracesExporterFlag.ToOutput(ctx).Untyped().(pulumi.StringOutput), | |
| }) | |
| } | |
| if traces.OtelTracesExporterJaegerEndpoint != nil { | |
| envVars = append(envVars, corev1.EnvVarArgs{ | |
| Name: pulumi.String("OTEL_TRACES_EXPORTER_JAEGER_ENDPOINT"), | |
| Value: traces.OtelTracesExporterJaegerEndpoint.ToOutput(ctx).Untyped().(pulumi.StringOutput), | |
| }) | |
| } | |
| if traces.OtelTracesExporterJaegerUser != nil { | |
| envVars = append(envVars, corev1.EnvVarArgs{ | |
| Name: pulumi.String("OTEL_TRACES_EXPORTER_JAEGER_USER"), | |
| Value: traces.OtelTracesExporterJaegerUser.ToOutput(ctx).Untyped().(pulumi.StringOutput), | |
| }) | |
| } | |
| if traces.OtelTracesExporterJaegerPassword != nil { | |
| envVars = append(envVars, corev1.EnvVarArgs{ | |
| Name: pulumi.String("OTEL_TRACES_EXPORTER_JAEGER_PASSWORD"), | |
| Value: pulumi.ToSecret(traces.OtelTracesExporterJaegerPassword.ToOutput(ctx).Untyped().(pulumi.StringOutput)), | |
| }) | |
| } | |
| if traces.OtelTracesExporterOTLPMode != nil { | |
| envVars = append(envVars, corev1.EnvVarArgs{ | |
| Name: pulumi.String("OTEL_TRACES_EXPORTER_OTLP_MODE"), | |
| Value: traces.OtelTracesExporterOTLPMode.ToOutput(ctx).Untyped().(pulumi.StringOutput), | |
| }) | |
| } | |
| if traces.OtelTracesExporterOTLPEndpoint != nil { | |
| envVars = append(envVars, corev1.EnvVarArgs{ | |
| Name: pulumi.String("OTEL_TRACES_EXPORTER_OTLP_ENDPOINT"), | |
| Value: traces.OtelTracesExporterOTLPEndpoint.ToOutput(ctx).Untyped().(pulumi.StringOutput), | |
| }) | |
| } | |
| if traces.OtelTracesExporterOTLPInsecure != nil { | |
| envVars = append(envVars, corev1.EnvVarArgs{ | |
| Name: pulumi.String("OTEL_TRACES_EXPORTER_OTLP_INSECURE"), | |
| Value: BoolToString(traces.OtelTracesExporterOTLPInsecure).Untyped().(pulumi.StringOutput), | |
| }) | |
| } | |
| } |
0c13bcf to
4ef49be
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
deployments/pulumi/pkg/worker/component.go (2)
24-29: 🛠️ Refactor suggestionUnused Service field
The
Serviceproperty onComponentis declared but never assigned a value in theNewComponentfunction, making it misleading.Either implement the service creation or remove this field to avoid confusion:
type Component struct { pulumi.ResourceState Deployment *appsv1.Deployment - Service *corev1.Service }
31-35:⚠️ Potential issueAdd validation for Database field
The
Databasefield is used on line 50 without verifying if it's nil, which could cause a panic.Add validation before accessing
Database.GetEnvVars():func NewComponent(ctx *pulumi.Context, name string, args ComponentArgs, opts ...pulumi.ResourceOption) (*Component, error) { cmp := &Component{} err := ctx.RegisterComponentResource("Formance:Ledger:Worker", name, cmp, opts...) if err != nil { return nil, err } + if args.Database == nil { + return nil, fmt.Errorf("database component is required but missing") + } envVars := corev1.EnvVarArray{}deployments/pulumi/pkg/api/service.go (1)
11-14:⚠️ Potential issueAdd validation for Deployment field
The
Deploymentfield is used on line 22 without verifying if it's nil, which could cause a panic if a nil deployment is passed.func createService(ctx *pulumi.Context, args createServiceArgs, opts ...pulumi.ResourceOption) (*corev1.Service, error) { + if args.Deployment == nil { + return nil, fmt.Errorf("deployment is required but missing") + } return corev1.NewService(ctx, "ledger", &corev1.ServiceArgs{deployments/pulumi/pkg/devbox/component.go (1)
18-21: 🛠️ Refactor suggestionConsider adding validation for Storage field.
The
ComponentArgsstruct contains aStoragefield that is later accessed directly without validation, which could lead to nil pointer dereferences if it's nil.Consider adding validation at the beginning of the
NewComponentfunction:func NewComponent(ctx *pulumi.Context, name string, args ComponentArgs, opts ...pulumi.ResourceOption) (*Component, error) { cmp := &Component{} err := ctx.RegisterComponentResource("Formance:Ledger:DevBox", name, cmp, opts...) if err != nil { return nil, err } + + if args.Storage == nil { + return nil, fmt.Errorf("storage component is required") + } + if args.Storage.Service == nil || args.Storage.Credentials == nil { + return nil, fmt.Errorf("storage service and credentials are required") + }deployments/pulumi/main.go (1)
22-28:⚠️ Potential issueAvoid exposing secrets in plaintext exports.
Exporting database credentials as plain text can pose a security risk. Consider using Pulumi secrets to avoid storing credentials unencrypted in state.
-ctx.Export("postgres-password", cmp.Storage.DatabaseComponent.GetPassword()) +ctx.Export("postgres-password", pulumi.ToSecret(cmp.Storage.DatabaseComponent.GetPassword()))
🧹 Nitpick comments (12)
deployments/pulumi/tools/main.go (2)
28-36: Remove commented code and consider more robust path handling.There's a commented-out option
//DoNotReference: true,and the hardcoded path forAddGoCommentscould be brittle if the repository structure changes.func printSchema(_ *cobra.Command, _ []string) error { reflector := jsonschema.Reflector{ - //DoNotReference: true, RequiredFromJSONSchemaTags: true, } - if err := reflector.AddGoComments("github.com/formancehq/ledger/deployments/pulumi", "./pkg"); err != nil { + // Use a more robust way to determine the package path + if err := reflector.AddGoComments("github.com/formancehq/ledger/deployments/pulumi", "./pkg"); err != nil { return err }
37-39: Consider pretty-printing the JSON output.For better readability of the schema output, especially when used for documentation, consider pretty-printing the JSON.
schema := reflector.Reflect(config.Config{}) - return json.NewEncoder(os.Stdout).Encode(schema) + encoder := json.NewEncoder(os.Stdout) + encoder.SetIndent("", " ") + return encoder.Encode(schema)deployments/pulumi/pkg/devbox/component.go (2)
30-48: Add affinity configuration for co-location with PostgreSQL.Since this devbox will interact with PostgreSQL, considering adding affinity rules to schedule the pod on the same node as the storage service for optimal performance.
Consider adding nodeAffinity or podAffinity to the deployment spec:
Spec: appsv1.DeploymentSpecArgs{ Selector: &metav1.LabelSelectorArgs{ MatchLabels: pulumi.StringMap{ "com.formance.stack/app": pulumi.String("ledger-devbox"), }, }, + // Add preferred pod affinity to co-locate with PostgreSQL pods + Template: &corev1.PodTemplateSpecArgs{ + Spec: corev1.PodSpecArgs{ + Affinity: &corev1.AffinityArgs{ + PodAffinity: &corev1.PodAffinityArgs{ + PreferredDuringSchedulingIgnoredDuringExecution: corev1.WeightedPodAffinityTermArray{ + corev1.WeightedPodAffinityTermArgs{ + Weight: pulumi.Int(100), + PodAffinityTerm: corev1.PodAffinityTermArgs{ + LabelSelector: &metav1.LabelSelectorArgs{ + MatchLabels: pulumi.StringMap{ + // Match the PostgreSQL pod labels + "app": args.Storage.Service.Metadata.Labels["app"], + }, + }, + TopologyKey: pulumi.String("kubernetes.io/hostname"), + }, + }, + }, + }, + },
106-108: Include meaningful outputs for the component.The current implementation registers empty outputs. Consider adding useful information about the component for better integration.
Add meaningful resource outputs:
- if err := ctx.RegisterResourceOutputs(cmp, pulumi.Map{}); err != nil { + if err := ctx.RegisterResourceOutputs(cmp, pulumi.Map{ + "deployment": cmp.Deployment.Metadata.Name(), + }); err != nil { return nil, fmt.Errorf("registering outputs: %w", err) }deployments/pulumi/pkg/api/component.go (2)
50-62: Consider adding error context for ingress creation.While the code checks for errors when creating the ingress, it doesn't add contextual information like it does for other resources.
Add error context for better debugging:
if args.Ingress != nil { if _, err := createIngress(ctx, createIngressArgs{ CommonArgs: args.CommonArgs, IngressArgs: IngressArgs{ Host: args.Ingress.Host, Secret: args.Ingress.Secret, Service: cmp.Service, }, }, pulumi.Parent(cmp), ); err != nil { - return nil, err + return nil, fmt.Errorf("creating ingress: %w", err) } }
64-66: Include meaningful outputs for the component.The current implementation registers empty outputs. Consider adding useful information about the component.
Add meaningful resource outputs:
-if err := ctx.RegisterResourceOutputs(cmp, pulumi.Map{}); err != nil { +if err := ctx.RegisterResourceOutputs(cmp, pulumi.Map{ + "deployment": cmp.Deployment.Metadata.Name(), + "service": cmp.Service.Metadata.Name(), +}); err != nil { return nil, fmt.Errorf("registering outputs: %w", err) }deployments/pulumi/pkg/storage/setup_postgres.go (1)
50-50: Remove or implement the placeholder.
SetDefaults()is currently empty. If no defaults are required, consider removing this function to avoid confusion, or fill in default logic if needed later.deployments/pulumi/pkg/utils/args_otel.go (1)
54-70: Refactor resource attributes string construction.
While manually joining with commas works, it can be simplified using built-in methods (e.g.,strings.Join) and improved for readability.- ret := "" - keys := collectionutils.Keys(rawResourceAttributes) - slices.Sort(keys) - for _, key := range keys { - ret += key + "=" + rawResourceAttributes[key] + "," - } - if len(ret) > 0 { - ret = ret[:len(ret)-1] - } - return ret + keys := collectionutils.Keys(rawResourceAttributes) + slices.Sort(keys) + pairs := []string{} + for _, key := range keys { + pairs = append(pairs, key+"="+rawResourceAttributes[key]) + } + return strings.Join(pairs, ",")deployments/pulumi/pkg/api/ingress.go (1)
23-24: Consider parameterizing the Ingress name.
Currently, the Ingress is named"ledger"on line 24. If multiple ledgers are ever deployed, or if more flexible naming is required, you might want to accept a parameter instead of a hard-coded value.deployments/pulumi/pkg/component.go (1)
27-32: Consider defaultingIngressfor consistency.
SetDefaults()applies defaults to several fields but omitsIngress. IfIngressis optional, that's fine. Otherwise, consider providing default values for consistency.deployments/pulumi/pkg/storage/component_rds.go (1)
63-65: Avoid forcing skipFinalSnapshot in production.
SettingSkipFinalSnapshottotruecan lead to data loss if resources are destroyed. You might want to parameterize this or default tofalsein production environments.deployments/pulumi/pkg/storage/component.go (1)
80-120: Consider consolidating environment variable logic.Repeatedly appending optional environment variables (e.g., MaxIdleConns, MaxOpenConns, etc.) in separate blocks can be refactored into a helper function that automatically appends these pairs if they're non-nil. This would reduce duplication and improve maintainability.
func (args *Component) GetEnvVars() corev1.EnvVarArray { ret := corev1.EnvVarArray{ // Existing environment variables... } - if args.ConnMaxIdleTime != nil { - ... - } - if args.MaxOpenConns != nil { - ... - } - if args.MaxIdleConns != nil { - ... - } + // Potential approach: factor out repeated logic into a helper function + appendIfNotNil := func(key string, value pulumix.Input[*time.Duration] or pulumix.Input[*int], transform func(...)) { + // If non-nil, append to ret with appropriate transformation + } + // Then just call appendIfNotNil(...) for each optional setting return ret }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
deployments/pulumi/docs/schema.jsonis excluded by!**/*.jsondeployments/pulumi/go.modis excluded by!**/*.moddeployments/pulumi/go.sumis excluded by!**/*.sum,!**/*.sumtools/k8s-stack/Pulumi.yamlis excluded by!**/*.yamltools/k8s-stack/go.modis excluded by!**/*.modtools/k8s-stack/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (31)
deployments/pulumi/README.md(1 hunks)deployments/pulumi/docs/schema.md(1 hunks)deployments/pulumi/justfile(1 hunks)deployments/pulumi/main.go(1 hunks)deployments/pulumi/main_test.go(2 hunks)deployments/pulumi/pkg/api/component.go(1 hunks)deployments/pulumi/pkg/api/deployment.go(9 hunks)deployments/pulumi/pkg/api/ingress.go(3 hunks)deployments/pulumi/pkg/api/service.go(2 hunks)deployments/pulumi/pkg/component.go(1 hunks)deployments/pulumi/pkg/component_args.go(0 hunks)deployments/pulumi/pkg/config/config.go(1 hunks)deployments/pulumi/pkg/devbox/component.go(1 hunks)deployments/pulumi/pkg/image.go(0 hunks)deployments/pulumi/pkg/storage/component.go(1 hunks)deployments/pulumi/pkg/storage/component_external.go(1 hunks)deployments/pulumi/pkg/storage/component_postgres.go(1 hunks)deployments/pulumi/pkg/storage/component_rds.go(1 hunks)deployments/pulumi/pkg/storage/migrate.go(2 hunks)deployments/pulumi/pkg/storage/setup_postgres.go(1 hunks)deployments/pulumi/pkg/storage/setup_rds.go(1 hunks)deployments/pulumi/pkg/utils/args_common.go(1 hunks)deployments/pulumi/pkg/utils/args_otel.go(1 hunks)deployments/pulumi/pkg/utils/convert.go(1 hunks)deployments/pulumi/pkg/worker/component.go(3 hunks)deployments/pulumi/tools/main.go(1 hunks)internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql(1 hunks)tools/k8s-stack/justfile(0 hunks)tools/k8s-stack/main.go(0 hunks)tools/k8s-stack/pkg/postgres.go(0 hunks)tools/k8s-stack/pkg/stack.go(0 hunks)
💤 Files with no reviewable changes (6)
- deployments/pulumi/pkg/image.go
- tools/k8s-stack/justfile
- tools/k8s-stack/main.go
- tools/k8s-stack/pkg/stack.go
- tools/k8s-stack/pkg/postgres.go
- deployments/pulumi/pkg/component_args.go
🚧 Files skipped from review as they are similar to previous changes (7)
- deployments/pulumi/README.md
- deployments/pulumi/pkg/utils/args_common.go
- deployments/pulumi/justfile
- deployments/pulumi/pkg/utils/convert.go
- deployments/pulumi/pkg/api/deployment.go
- internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql
- deployments/pulumi/pkg/storage/component_external.go
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
deployments/pulumi/docs/schema.md
5-5: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
16-16: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
36-36: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
57-57: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
72-72: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
85-85: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
100-100: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
117-117: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
131-131: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
146-146: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
160-160: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
174-174: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
187-187: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
206-206: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
219-219: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
232-232: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (40)
deployments/pulumi/pkg/storage/migrate.go (3)
16-24: LGTM! Good environment variable setup.The environment variable setup is clean and correctly integrates the DEBUG flag with component environment variables.
39-42: The image retrieval and environment setup looks good.Using
utils.GetImageprovides a consistent way to retrieve images across components.
47-48: Good use of DeleteBeforeReplace option.Adding
pulumi.DeleteBeforeReplace(true)ensures clean resource updates by removing the old resource before creating a new one, which is especially important for migration jobs.deployments/pulumi/pkg/worker/component.go (4)
50-53: LGTM! Good conditional handling of Otel environment variables.The code correctly checks if
args.Otelis nil before accessing its methods.
79-81: Add validation for the image tagThe code calls
utils.GetImage(args.Tag)without validatingargs.Tag, which could lead to using an invalid or insecure image.func NewComponent(ctx *pulumi.Context, name string, args ComponentArgs, opts ...pulumi.ResourceOption) (*Component, error) { // ... existing code ... + if args.Tag == "" { + return nil, fmt.Errorf("image tag is required but empty") + } // ... continue with deployment creation ...#!/bin/bash # Check if utils.GetImage function has internal validation # Look for the GetImage implementation rg -p "func GetImage" -g "*.go" --context=3
92-94: Great error handling improvement.Adding specific error context with
fmt.Errorf("creating deployment: %w", err)improves debugging and makes error messages more informative.
96-98: Good proper resource registration.Properly registering resource outputs and wrapping the error with context is a best practice in Pulumi.
deployments/pulumi/tools/main.go (1)
12-26: LGTM! Well-structured CLI implementation.The Cobra command setup is clean and follows the standard pattern for CLI tools. Error handling is properly implemented with appropriate exit codes.
deployments/pulumi/pkg/api/service.go (2)
22-23: Good refactoring using Deployment Spec Selector.Using
args.Deployment.Spec.Selector().MatchLabels()directly ensures that the service selector always matches the deployment, which is a best practice.
33-34: LGTM! Clean implementation of variadic options.Passing the
optsvariadic parameter correctly to the underlying resource creation function is a good practice.deployments/pulumi/pkg/devbox/component.go (1)
65-65: Avoid infinite sleep in production deployments.The container runs indefinitely with
sleep infinity, which is suitable for development scenarios but risky in production. Consider adding proper health checks, readiness probes, or a minimal process that terminates gracefully.deployments/pulumi/main_test.go (4)
14-17: Updated test case structure to support complex configuration.The change from
map[string]stringtomap[string]anyallows for more structured configuration which is good. This will better support nested configuration objects.
21-28: Test configuration uses realistic values.The test configuration now uses more realistic values including an integer for timeout and a nested structure for storage configuration, which will better match real-world usage.
32-39: Well-handled JSON marshaling for configuration values.The approach of marshaling complex values to JSON strings is a good way to handle the conversion from structured config to the string map expected by Pulumi.
39-39: Ensure namespace uniqueness for test isolation.The use of a UUID to generate a unique namespace for each test run helps ensure that tests don't interfere with each other, which is good practice.
deployments/pulumi/main.go (2)
10-15: Simplified Pulumi program structure.The simplified structure with error handling looks good. Delegating configuration loading to a dedicated function improves code organization.
17-20: Streamlined component creation.The component creation is now more concise with better error propagation, which is a good improvement.
deployments/pulumi/pkg/api/component.go (4)
12-17: Well-structured Component with clear resource references.The Component struct design is clean and provides clear access to the underlying resources.
19-24: ComponentArgs includes all necessary parameters.The ComponentArgs struct effectively encapsulates all the required configuration, which is good design.
26-31: Good resource registration pattern.The code follows the recommended Pulumi pattern of registering the component resource before creating child resources, with proper error handling.
33-40: Proper error wrapping for deployment creation.The error handling with contextual information will make debugging easier, which is good practice.
deployments/pulumi/pkg/storage/setup_postgres.go (3)
41-42: Avoid panics in Pulumi code.
Usingpanic("uri must be provided if install is false")can abruptly terminate the provisioning process. Consider returning an error instead for smoother error handling and debug logs, consistent with best practices in Pulumi.
45-48: Confirm resource naming consistency.
The hard-coded"postgres"string is used as the resource name here. Verify that other Postgres resources also use consistent naming across the codebase.
17-35:Details
❓ Verification inconclusive
Validate DSN fallback handling.
When theURIis provided, the code correctly parses and extracts the port. Consider verifying ifdsnparser.Parse()consistently handles missing or invalid port values, and confirm that the code can gracefully handle alternate DSN formats.As a follow-up, you can run a shell script to search for all DSN references and check if they might provide port-less URIs:
🏁 Script executed:
#!/bin/bash # Searching for DSN usage within the repository to confirm consistent handling rg -A 5 'dsnparser\.Parse'Length of output: 569
DSN Fallback Handling Verification – Please Confirm
I've verified that the only usage of
dsnparser.Parse()in the repository is indeployments/pulumi/pkg/storage/setup_postgres.go(lines 17–35). The current implementation parses the URI and extracts the port usingstrconv.Atoi(dsn.GetPort()), and an error is returned if port parsing fails. However, there is no fallback mechanism or explicit handling for DSNs that might be missing a port or have an invalid port format.
- Action Required:
- Confirm that
dsnparser.Parse()internally supports graceful handling of alternate DSN formats (e.g., DSNs without a port) or consider adding a fallback/default port value if needed.- Review and test DSN inputs with missing or invalid port values to ensure robust behavior.
deployments/pulumi/pkg/utils/args_otel.go (3)
90-101: Use secrets for Jaeger credentials.
Exporting Jaeger user/password as plaintext environment variables risks potential credential leakage. Consider marking them as Pulumi secrets or employing more secure injection methods.
122-133: Handle invalid push interval.
The code gracefully handlesnilintervals. Confirm that the resulting empty string for invalid intervals is acceptable downstream. If the environment variable is mandatory, warn or default to a sane value to avoid misconfiguration.
140-149: Check for nil intervals in metrics runtime.
Similar to push interval, ensure that setting an empty string whenOtelMetricsRuntimeMinimumReadMemStatsIntervalisnildoes not cause unintended behavior in the metrics aggregator.deployments/pulumi/pkg/api/ingress.go (1)
50-70: Graceful handling of HTTP-only Ingress.
The inlineApplylogic properly returnsnilTLS config ifSecretis empty, enabling HTTP-only mode. This approach is good; just confirm that other parts of the deployment handle plain HTTP gracefully.deployments/pulumi/pkg/component.go (3)
13-13: Verify usage of internal Pulumi package.
Relying onpulumi/sdk/v3/go/pulumi/internalscan be risky, as it may change without warning. Please double-check that its usage is recommended and stable for production.
44-129: Break down theNewComponentfunction for clarity.
This function creates multiple resources (namespace, storage, API, worker, devbox) in a single block. For better readability and maintainability, consider splitting it into logical subfunctions.
108-112: Add error handling to the type assertion forinstallDevBox.
Currently, the code usesinstallDevBox.Value.(bool)without verifying the type. If this value isn't a boolean, a panic will occur.deployments/pulumi/pkg/storage/setup_rds.go (2)
21-74: Default credentials can pose security risks.
Defining"root"and"password"inSetDefaults()allows for accidental use in production. Use environment variables or a secure secret manager to supply strong credentials.
117-145: Verify consistency of existing cluster password.
When connecting to an existing RDS cluster, there's no check that the providedMasterPasswordmatches what’s actually configured. This could lead to credential mismatches. Consider verifying correctness, if applicable.deployments/pulumi/pkg/storage/component_postgres.go (1)
63-64: Avoid hard-coded default credentials.
Storing"root"and"password"in source code is insecure. Externalize them or generate strong credentials at runtime to reduce exposure risk.deployments/pulumi/pkg/storage/component.go (1)
168-270: Looks good overall.The
NewComponentfunction is initialized properly, with clear error handling and resource registration. The approach of deferring migrations until after the component is created is logically consistent, and there are no concerns about concurrency.deployments/pulumi/pkg/config/config.go (5)
148-150: Avoid usingpanicfor missing configuration.
Panicking can disrupt Pulumi previews and partial deployments. Instead, return an error to gracefully stop.-if !a.Install { - panic("uri must be provided if install is false") -} +if !a.Install { + return nil, fmt.Errorf("uri must be provided if install is false") +}
85-87: Add nil checks for CreateCluster and UseCluster.
Accessinga.CreateCluster.toInput()ora.UseCluster.toInput()directly might panic if either is nil.func (a *RDSDatabase) toInput() *storage.RDSDatabaseArgs { if a == nil { return nil } return &storage.RDSDatabaseArgs{ - CreateCluster: a.CreateCluster.toInput(), - UseCluster: a.UseCluster.toInput(), + CreateCluster: func() *storage.RDSClusterCreateArgs { + if a.CreateCluster == nil { + return nil + } + return a.CreateCluster.toInput() + }(), + UseCluster: func() *storage.RDSUseExistingClusterArgs { + if a.UseCluster == nil { + return nil + } + return a.UseCluster.toInput() + }(), PostMigrateSnapshot: a.PostMigrateSnapshot.toInput(), } }
316-326: Add nil checks for Jaeger and OTLP to prevent nil pointers.
Ift.Jaegerort.OTLPis nil, accessing their fields directly can lead to a panic.func (t *OtelTraces) toInput() *utils.OtelTracesArgs { if t == nil { return nil } + if t.Jaeger == nil { + t.Jaeger = &OtelTracesJaeger{} + } + if t.OTLP == nil { + t.OTLP = &OtelTracesOTLP{} + } return &utils.OtelTracesArgs{ OtelTracesBatch: pulumix.Val(t.Batch), OtelTracesExporterFlag: pulumix.Val(t.ExporterFlag), OtelTracesExporterJaegerEndpoint: pulumix.Val(t.Jaeger.Endpoint), OtelTracesExporterJaegerUser: pulumix.Val(t.Jaeger.User), OtelTracesExporterJaegerPassword: pulumix.Val(t.Jaeger.Password), OtelTracesExporterOTLPMode: pulumix.Val(t.OTLP.Mode), OtelTracesExporterOTLPEndpoint: pulumix.Val(t.OTLP.Endpoint), OtelTracesExporterOTLPInsecure: pulumix.Val(t.OTLP.Insecure), } }
359-373: Prevent potential nil pointer dereference for OtelMetricsOTLP.
Callingm.OtelMetricsOTLP.Modeetc. without checking ifm.OtelMetricsOTLPis nil can result in a panic.func (m *OtelMetrics) toInput() *utils.OtelMetricsArgs { if m == nil { return nil } + safeOTLP := &OtelMetricsOTLP{} + if m.OtelMetricsOTLP != nil { + safeOTLP = m.OtelMetricsOTLP + } return &utils.OtelMetricsArgs{ OtelMetricsExporterPushInterval: pulumix.Val(m.PushInterval), OtelMetricsRuntime: pulumix.Val(m.Runtime), OtelMetricsRuntimeMinimumReadMemStatsInterval: pulumix.Val(m.RuntimeMinimumReadMemStatsInterval), OtelMetricsExporter: pulumix.Val(m.Exporter), OtelMetricsKeepInMemory: pulumix.Val(m.KeepInMemory), - OtelMetricsExporterOTLPMode: pulumix.Val(m.OtelMetricsOTLP.Mode), - OtelMetricsExporterOTLPEndpoint: pulumix.Val(m.OtelMetricsOTLP.Endpoint), - OtelMetricsExporterOTLPInsecure: pulumix.Val(m.OtelMetricsOTLP.Insecure), + OtelMetricsExporterOTLPMode: pulumix.Val(safeOTLP.Mode), + OtelMetricsExporterOTLPEndpoint: pulumix.Val(safeOTLP.Endpoint), + OtelMetricsExporterOTLPInsecure: pulumix.Val(safeOTLP.Insecure), } }
385-386: Validate image pull policy.
Consider explicitly validatingc.ImagePullPolicyso that only valid Kubernetes values (e.g.,"Always","IfNotPresent","Never") are accepted. This helps prevent invalid configurations from causing deployment issues down the line.func (c Common) toInput() utils.CommonArgs { + validPolicies := map[string]bool{"Always": true, "IfNotPresent": true, "Never": true} + if !validPolicies[c.ImagePullPolicy] && c.ImagePullPolicy != "" { + // Return or log error here to prevent invalid configurations + // Or fallback to a default policy if desired + } return utils.CommonArgs{ Namespace: pulumix.Val(c.Namespace), Otel: c.Monitoring.ToInput(), Tag: pulumix.Val(c.Tag), ImagePullPolicy: pulumix.Val(c.ImagePullPolicy), Debug: pulumix.Val(c.Debug), } }
| type migrationArgs struct { | ||
| utils.CommonArgs | ||
| component *Component | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for component to prevent nil pointer dereference.
The migrationArgs struct has a component field which is used in runMigrateJob without validation. This could cause a nil pointer dereference if a nil component is passed.
func runMigrateJob(ctx *pulumi.Context, args migrationArgs, opts ...pulumi.ResourceOption) (*batchv1.Job, error) {
+ if args.component == nil {
+ return nil, fmt.Errorf("component is required but missing")
+ }
envVars := corev1.EnvVarArray{
corev1.EnvVarArgs{
Name: pulumi.String("DEBUG"),
Value: utils.BoolToString(args.Debug).Untyped().(pulumi.StringOutput),
},
}
envVars = append(envVars, args.component.GetEnvVars()...)📝 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.
| type migrationArgs struct { | |
| utils.CommonArgs | |
| component *Component | |
| } | |
| func runMigrateJob(ctx *pulumi.Context, args migrationArgs, opts ...pulumi.ResourceOption) (*batchv1.Job, error) { | |
| if args.component == nil { | |
| return nil, fmt.Errorf("component is required but missing") | |
| } | |
| envVars := corev1.EnvVarArray{ | |
| corev1.EnvVarArgs{ | |
| Name: pulumi.String("DEBUG"), | |
| Value: utils.BoolToString(args.Debug).Untyped().(pulumi.StringOutput), | |
| }, | |
| } | |
| envVars = append(envVars, args.component.GetEnvVars()...) | |
| // ... remaining implementation of runMigrateJob | |
| } |
| pulumi.String(` | ||
| #!/bin/sh | ||
|
|
||
| apk update | ||
| apk add postgresql-client httpie bash | ||
|
|
||
| sleep infinity | ||
| `), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to container shell script.
The shell script runs commands but doesn't fail if they encounter errors, which could lead to a partially configured container.
Add set -e to the shell script to fail on any command errors:
pulumi.String(`
- #!/bin/sh
+ #!/bin/sh
+ set -e
apk update
apk add postgresql-client httpie bash
sleep infinity
`),📝 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.
| pulumi.String(` | |
| #!/bin/sh | |
| apk update | |
| apk add postgresql-client httpie bash | |
| sleep infinity | |
| `), | |
| pulumi.String(` | |
| #!/bin/sh | |
| set -e | |
| apk update | |
| apk add postgresql-client httpie bash | |
| sleep infinity | |
| `), |
| Name: pulumi.String("PGPASSWORD"), | ||
| Value: pulumi.String("$(POSTGRES_PASSWORD)"), | ||
| }, |
There was a problem hiding this comment.
Fix environment variable expansion issue.
Kubernetes environment variables don't support shell-style variable expansion with $(). This needs to be handled differently.
Replace the PGPASSWORD environment variable with a direct reference to the secret:
-corev1.EnvVarArgs{
- Name: pulumi.String("PGPASSWORD"),
- Value: pulumi.String("$(POSTGRES_PASSWORD)"),
-},
+corev1.EnvVarArgs{
+ Name: pulumi.String("PGPASSWORD"),
+ ValueFrom: corev1.EnvVarSourceArgs{
+ SecretKeyRef: &corev1.SecretKeySelectorArgs{
+ Key: pulumi.String("password"),
+ Name: args.Storage.Credentials.Metadata.Name(),
+ },
+ },
+},📝 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.
| Name: pulumi.String("PGPASSWORD"), | |
| Value: pulumi.String("$(POSTGRES_PASSWORD)"), | |
| }, | |
| corev1.EnvVarArgs{ | |
| Name: pulumi.String("PGPASSWORD"), | |
| ValueFrom: corev1.EnvVarSourceArgs{ | |
| SecretKeyRef: &corev1.SecretKeySelectorArgs{ | |
| Key: pulumi.String("password"), | |
| Name: args.Storage.Credentials.Metadata.Name(), | |
| }, | |
| }, | |
| }, |
| Cluster *rds.Cluster | ||
| Instance *rds.ClusterInstance | ||
| } |
There was a problem hiding this comment.
Assign the Instance field properly.
The code defines Instance *rds.ClusterInstance in the struct, but the returned instance is never assigned, leaving the field nil. This may break downstream references or intended usage.
Apply this patch to store the reference:
- _, err = rds.NewClusterInstance(ctx, "primary", &rds.ClusterInstanceArgs{
+ cmp.Instance, err = rds.NewClusterInstance(ctx, "primary", &rds.ClusterInstanceArgs{
ClusterIdentifier: cmp.Cluster.ClusterIdentifier,
InstanceClass: ...Also applies to: 90-102
|
|
||
| JSON Schema missing a description, provide it using the `description` key in the root of the JSON document. | ||
|
|
||
| ### Type: `object(?)` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust heading levels to comply with markdownlint rules.
The tool flags that headings should only increment by one level at a time (MD001). Updating these headings ensures consistent structure and avoids pipeline failures.
Below is an illustrative fix for a few lines. Apply similarly for all mismatched heading levels:
-### Type: `object(?)`
+## Type: `object(?)`
-#### Type: `object`
+### Type: `object`Also applies to: 16-16, 36-36, 57-57, 72-72, 85-85, 100-100, 117-117, 131-131, 146-146, 160-160, 174-174, 187-187, 206-206, 219-219, 232-232
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
5-5: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
2 participants
No description provided.