fix: pulumi postgres chart version #1140
Conversation
WalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
deployments/pulumi/pkg/config/config.go (1)
188-188: Consider validating non-emptyChartVersionbefore passing to Pulumi.While the field is correctly propagated through
toInput(), passing an empty string forChartVersioncould cause unexpected behavior in Helm chart deployment. Consider adding validation or ensuring a default is set upstream.If validation is desired at this level, you could add:
return &storage.PostgresDatabaseArgs{ Install: &storage.PostgresInstallArgs{ Username: pulumix.Val(a.Install.Username), Password: pulumix.Val(a.Install.Password), - ChartVersion: pulumix.Val(a.Install.ChartVersion), + ChartVersion: pulumix.Val(func() string { + if a.Install.ChartVersion == "" { + return "16.4.7" // or appropriate default + } + return a.Install.ChartVersion + }()), }, }However, since the
SetDefaults()method incomponent_postgres.gois the more appropriate place for this logic (as suggested in my other comment), you may choose to handle defaults there instead.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
deployments/pulumi/pkg/config/config.go(2 hunks)deployments/pulumi/pkg/storage/component_postgres.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (1)
deployments/pulumi/pkg/config/config.go (1)
157-159: LGTM! Field properly added with JSON/YAML tags.The
ChartVersionfield is correctly added to thePostgresInstallstruct with appropriate serialization tags.
| type PostgresInstallArgs struct { | ||
| Username pulumix.Input[string] | ||
| Password pulumix.Input[string] | ||
| ChartVersion pulumix.Input[string] |
There was a problem hiding this comment.
Add default value for ChartVersion in SetDefaults() method.
The new ChartVersion field lacks a default value. When ChartVersion is nil or not provided, line 101 will panic during the type assertion.
Add default handling in the SetDefaults() method (after line 82):
return password
})
+ if args.ChartVersion == nil {
+ args.ChartVersion = pulumix.Val("")
+ }
+ args.ChartVersion = pulumix.Apply(args.ChartVersion, func(version string) string {
+ if version == "" {
+ return "16.4.7" // or another appropriate default version
+ }
+ return version
+ })
}📝 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.
| ChartVersion pulumix.Input[string] | |
| return password | |
| }) | |
| if args.ChartVersion == nil { | |
| args.ChartVersion = pulumix.Val("") | |
| } | |
| args.ChartVersion = pulumix.Apply(args.ChartVersion, func(version string) string { | |
| if version == "" { | |
| return "16.4.7" // or another appropriate default version | |
| } | |
| return version | |
| }) | |
| } |
🤖 Prompt for AI Agents
In deployments/pulumi/pkg/storage/component_postgres.go around line 61 (and
modify SetDefaults() after line 82), the ChartVersion pulumix.Input[string]
field has no default and a nil value causes a panic at the type assertion on
line 101; update SetDefaults() to check if ChartVersion is nil or empty and
assign a sensible default chart version (e.g., a constant default string) before
any type assertions or use, ensuring subsequent code can safely assume
ChartVersion is non-nil.
| cmp.Chart, err = helm.NewChart(ctx, "postgres", &helm.ChartArgs{ | ||
| Chart: pulumi.String("oci://registry-1.docker.io/bitnamicharts/postgresql"), | ||
| Version: pulumi.String("16.4.7"), | ||
| Version: args.ChartVersion.ToOutput(ctx.Context()).Untyped().(pulumi.StringOutput), |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the type assertion safety.
The type assertion .Untyped().(pulumi.StringOutput) will panic if ChartVersion is nil. Once the default value is added in SetDefaults() (as suggested for line 61), this will be safe.
Run the following script to verify all callers of newPostgresComponent properly initialize PostgresInstallArgs:
🏁 Script executed:
#!/bin/bash
# Description: Verify all instantiations of PostgresInstallArgs include proper initialization
# Search for PostgresInstallArgs struct instantiations
ast-grep --pattern 'PostgresInstallArgs{$$$}'
# Search for calls to newPostgresComponent to verify SetDefaults is called
rg -n -A5 -B5 'newPostgresComponent'Length of output: 2030
I need to gather more information to verify the type assertion safety. Let me examine the SetDefaults() method and the complete initialization flow.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find SetDefaults method for PostgresInstallArgs
rg -n 'func.*SetDefaults' -A 10 deployments/pulumi/pkg/storage/
# View the full setup_postgres.go to understand initialization order
cat deployments/pulumi/pkg/storage/setup_postgres.go
Length of output: 7153
I need to see the complete SetDefaults() method for PostgresInstallArgs to verify if ChartVersion gets a default value.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full SetDefaults method for PostgresInstallArgs and view ChartVersion field definition
cat deployments/pulumi/pkg/storage/component_postgres.go | head -85
Length of output: 2088
Now I need to verify the type assertion at line 101 and see if ChartVersion is actually being used there.
---
🏁 Script executed:
```shell
#!/bin/bash
# View lines 95-110 to see the ChartVersion assertion at line 101
sed -n '95,110p' deployments/pulumi/pkg/storage/component_postgres.go
Length of output: 574
Let me verify if ChartVersion gets a default elsewhere or if this is truly missing.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for ChartVersion references and defaults
rg -n 'ChartVersion' deployments/pulumi/pkg/storage/
# Check if there are multiple SetDefaults() calls or if ChartVersion is set in constructor
rg -n 'ChartVersion.*=' deployments/pulumi/pkg/storage/
Length of output: 236
Add default for ChartVersion in PostgresInstallArgs.SetDefaults().
The type assertion at line 101 will panic if ChartVersion is nil. The SetDefaults() method sets defaults for Username and Password but does not initialize ChartVersion, leaving it null and causing a panic at the assertion. Add a default (e.g., "" or a minimum supported version) in SetDefaults() at component_postgres.go:64-81.
🤖 Prompt for AI Agents
In deployments/pulumi/pkg/storage/component_postgres.go around line 101 (and
update SetDefaults() at ~lines 64-81), the code type-asserts
args.ChartVersion.ToOutput(...).Untyped().(pulumi.StringOutput) which will panic
if ChartVersion is nil; modify PostgresInstallArgs.SetDefaults() to initialize
ChartVersion when nil (e.g., set to an empty string "" or a specified minimum
supported version) so the field is non-nil and the subsequent ToOutput/Untyped
type assertion succeeds; ensure the default is of the same type used elsewhere
and keep existing defaulting for Username/Password.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/v2.3 #1140 +/- ##
================================================
- Coverage 81.90% 81.88% -0.03%
================================================
Files 187 187
Lines 8924 8924
================================================
- Hits 7309 7307 -2
- Misses 1188 1191 +3
+ Partials 427 426 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.