test: add rolling upgrades tests #895
Conversation
WalkthroughThis set of changes introduces a new rolling upgrades CLI tool under Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (rolling-upgrades)
participant Pulumi Automation API
participant Pulumi Stack
User->>CLI (rolling-upgrades): Execute with flags (init, target, stack, etc.)
CLI (rolling-upgrades)->>CLI (rolling-upgrades): Load and merge configs
CLI (rolling-upgrades)->>Pulumi Automation API: Create/Select Stack
CLI (rolling-upgrades)->>Pulumi Stack: Destroy generator resource (if exists)
CLI (rolling-upgrades)->>Pulumi Stack: Up stack with initial config
CLI (rolling-upgrades)->>Pulumi Stack: Up stack with target config (replace resource)
CLI (rolling-upgrades)->>Pulumi Stack: Optionally destroy all resources (cleanup)
Pulumi Stack-->>CLI (rolling-upgrades): Operation results
CLI (rolling-upgrades)-->>User: Output results
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
4961558 to
fc715e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
test/rolling-upgrades/cmd/script.js (1)
6-15: Consider exportingnextto improve re-usabilityIf this script is ever
required/imported as a module (e.g. by k6 or a node runner) thenextfunction will be out of scope.
Adding an explicit export future-proofs the file without changing current behaviour when it’s only embedded as plain text.function next() { return [{ action: 'CREATE_TRANSACTION', data: { script: { plain } } }] } + +module.exports = { next };deployments/pulumi/pkg/config/config.go (1)
256-262: Interface conformity checks are great – tiny tweakThe “dummy value” assertions are valuable, but using a pointer for the interfaces that require a pointer receiver (Unmarshal) is clearer:
-var _ json.Unmarshaler = (*Duration)(nil) +var _ json.Unmarshaler = (*Duration)(nil) // already okNothing blocking, just a minor style note.
test/rolling-upgrades/cmd/root.go (2)
99-107: Unnecessary file handle opened and never used.
os.Create(tmpConfigFileName)is invoked here, yet the file is reopened insideupdateConfigFile().
The first handle is kept open until the end ofrunE, wasting an FD and risking platform-specific file-locking issues.You can safely remove lines 100-106 and rely on
updateConfigFile()to create/truncate the file.
72-73:--toggle/-tflag appears to be a leftover sample flag.It is not read anywhere in the codebase and may confuse users of the CLI.
Consider deleting it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.github/workflows/rolling-upgrades.ymlis excluded by!**/*.ymldeployments/pulumi/go.modis excluded by!**/*.modtest/rolling-upgrades/go.modis excluded by!**/*.modtest/rolling-upgrades/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (6)
Justfile(2 hunks)deployments/pulumi/main.go(0 hunks)deployments/pulumi/pkg/config/config.go(14 hunks)test/rolling-upgrades/cmd/root.go(1 hunks)test/rolling-upgrades/cmd/script.js(1 hunks)test/rolling-upgrades/main.go(1 hunks)
💤 Files with no reviewable changes (1)
- deployments/pulumi/main.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/rolling-upgrades/main.go (1)
test/rolling-upgrades/cmd/root.go (1)
Execute(57-62)
test/rolling-upgrades/cmd/root.go (2)
deployments/pulumi/pkg/config/config.go (8)
Common(500-515)Storage(233-244)API(299-326)Worker(342-342)Config(611-637)Generator(592-598)GeneratorLedgerConfiguration(561-579)Duration(256-256)pkg/client/internal/utils/utils.go (1)
Contains(62-69)
🔇 Additional comments (3)
Justfile (2)
17-18: Nice addition – keeps new code covered by lintingIncluding the new directory in the lint pass is spot-on and consistent with the existing pattern.
No issues spotted here.
28-29: Tidy target update looks goodEnsures module dependencies of the new package stay clean.
Thanks for keeping the workflow consistent.deployments/pulumi/pkg/config/config.go (1)
304-305: Zero vs omitted: consider using a pointer
API.GracePeriodis now a value (zero default).
Callers cannot distinguish “not provided” from “explicit 0”.
If 0 is a meaningful value, keep as-is; otherwise a*Durationwithomitemptywould allow intent to be preserved.No action required if 0 is indeed the intended default.
| import "github.com/formance/ledger/tests/rolling-upgrades/cmd" | ||
|
|
||
| func main() { | ||
| cmd.Execute() |
There was a problem hiding this comment.
Incorrect module path will break compilation
The import path lacks the hq segment used everywhere else in the repo and points to tests/rolling-upgrades, while the actual directory is test/rolling-upgrades.
This will cause go vet, go build, and golangci-lint to fail.
-import "github.com/formance/ledger/tests/rolling-upgrades/cmd"
+import "github.com/formancehq/ledger/test/rolling-upgrades/cmd"📝 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.
| import "github.com/formance/ledger/tests/rolling-upgrades/cmd" | |
| func main() { | |
| cmd.Execute() | |
| import "github.com/formancehq/ledger/test/rolling-upgrades/cmd" | |
| func main() { | |
| cmd.Execute() |
🤖 Prompt for AI Agents (early access)
In test/rolling-upgrades/main.go around lines 3 to 6, the import path is incorrect; it should include the 'hq' segment and use 'test/rolling-upgrades' instead of 'tests/rolling-upgrades'. Update the import statement to the correct module path reflecting the actual directory structure and consistent with the rest of the repo to fix compilation and linting errors.
| func (d *Duration) UnmarshalJSON(data []byte) error { | ||
|
|
||
| fmt.Println(string(data)) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove stray debug fmt.Println
Leaving unconditional prints in a library function will spam stdout for every config load, including in production CI/CD runs.
- fmt.Println(string(data))📝 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 (d *Duration) UnmarshalJSON(data []byte) error { | |
| fmt.Println(string(data)) | |
| func (d *Duration) UnmarshalJSON(data []byte) error { |
🤖 Prompt for AI Agents (early access)
In deployments/pulumi/pkg/config/config.go around lines 263 to 266, remove the stray debug fmt.Println statement that prints the JSON data during unmarshalling. This print causes unnecessary output in production and CI/CD environments. Simply delete or comment out the fmt.Println line to prevent spamming stdout on every config load.
| overlays, err := cmd.Flags().GetStringArray(overlayFlag) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| for _, overlay := range overlays { | ||
| oCfg := &PartialConfig{} | ||
| if err := json.Unmarshal([]byte(overlay), oCfg); err != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal overlay (%s): %w", overlay, err) | ||
| } | ||
|
|
||
| err := mergo.Merge(cfg, oCfg) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to merge overlay (%s): %w", overlay, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Overlay values never override base config – pass mergo.WithOverride (and preserve ordering).
mergo.Merge(dst, src) only populates empty fields by default, so later overlays cannot replace previously set values.
This effectively disables the whole “overlay” concept for non-zero fields and makes the result order-dependent in an unintuitive way.
- err := mergo.Merge(cfg, oCfg)
+ // Allow overlay values to override existing ones.
+ err := mergo.Merge(cfg, oCfg, mergo.WithOverride)You may also want to sort / apply the overlays in the order they were supplied so --overlay A --overlay B gives B the highest priority.
Without this fix, users will get silently-ignored overrides.
📝 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.
| overlays, err := cmd.Flags().GetStringArray(overlayFlag) | |
| if err != nil { | |
| return nil, err | |
| } | |
| for _, overlay := range overlays { | |
| oCfg := &PartialConfig{} | |
| if err := json.Unmarshal([]byte(overlay), oCfg); err != nil { | |
| return nil, fmt.Errorf("failed to unmarshal overlay (%s): %w", overlay, err) | |
| } | |
| err := mergo.Merge(cfg, oCfg) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to merge overlay (%s): %w", overlay, err) | |
| } | |
| } | |
| overlays, err := cmd.Flags().GetStringArray(overlayFlag) | |
| if err != nil { | |
| return nil, err | |
| } | |
| for _, overlay := range overlays { | |
| oCfg := &PartialConfig{} | |
| if err := json.Unmarshal([]byte(overlay), oCfg); err != nil { | |
| return nil, fmt.Errorf("failed to unmarshal overlay (%s): %w", overlay, err) | |
| } | |
| // Allow overlay values to override existing ones. | |
| err := mergo.Merge(cfg, oCfg, mergo.WithOverride) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to merge overlay (%s): %w", overlay, err) | |
| } | |
| } |
🤖 Prompt for AI Agents (early access)
In test/rolling-upgrades/cmd/root.go around lines 274 to 288, the current use of mergo.Merge does not allow overlay values to override existing fields because it lacks the mergo.WithOverride option. To fix this, update the mergo.Merge call to include mergo.WithOverride so that overlay fields replace existing values. Also, ensure overlays are applied in the order they are supplied so that later overlays have higher priority, preserving the intended override behavior.
| if err := upStack(cmd, stack, tmpConfigFileName, | ||
| optup.Replace([]string{"urn:pulumi:tests-rolling-upgrades::ledger::Formance:Ledger$Formance:Ledger:API$kubernetes:apps/v1:Deployment::ledger-api"}), | ||
| ); err != nil { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
optup.Replace URN also hard-codes the stack name.
Same issue as above – if the user passes --stack-name foo, the replacement will silently do nothing.
- optup.Replace([]string{"urn:pulumi:tests-rolling-upgrades::ledger::Formance:Ledger$Formance:Ledger:API$kubernetes:apps/v1:Deployment::ledger-api"}),
+ optup.Replace([]string{
+ fmt.Sprintf("urn:pulumi:%s::ledger::Formance:Ledger$Formance:Ledger:API$kubernetes:apps/v1:Deployment::ledger-api", stack.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.
| if err := upStack(cmd, stack, tmpConfigFileName, | |
| optup.Replace([]string{"urn:pulumi:tests-rolling-upgrades::ledger::Formance:Ledger$Formance:Ledger:API$kubernetes:apps/v1:Deployment::ledger-api"}), | |
| ); err != nil { | |
| if err := upStack(cmd, stack, tmpConfigFileName, | |
| optup.Replace([]string{ | |
| fmt.Sprintf("urn:pulumi:%s::ledger::Formance:Ledger$Formance:Ledger:API$kubernetes:apps/v1:Deployment::ledger-api", stack.Name()), | |
| }), | |
| ); err != nil { |
🤖 Prompt for AI Agents (early access)
In test/rolling-upgrades/cmd/root.go around lines 151 to 153, the optup.Replace call hard-codes the stack name in the URN, causing it to ignore any user-supplied --stack-name argument. Modify the URN string construction to dynamically include the actual stack name variable instead of the fixed "tests-rolling-upgrades" value, ensuring the replacement works correctly for any stack name provided by the user.
| _, err = stack.Destroy(cmd.Context(), destroyOptions...) | ||
| // Ugly check... | ||
| if err != nil && !strings.Contains(err.Error(), "no resource named") { | ||
| return err | ||
| } | ||
| if strings.Contains(err.Error(), "no resource named") { | ||
| pterm.Info.Println("No generator to destroy.") | ||
| } else { | ||
| pterm.Info.Println("Generator destroyed.") |
There was a problem hiding this comment.
Possible nil-pointer panic when err is nil.
err.Error() is called even when the previous Destroy succeeded (err == nil).
A successful destroy will therefore panic the CLI.
- if err != nil && !strings.Contains(err.Error(), "no resource named") {
- return err
- }
- if strings.Contains(err.Error(), "no resource named") {
+ if err != nil {
+ if !strings.Contains(err.Error(), "no resource named") {
+ return err
+ }
+ // err is non-nil and indicates “no resource named”
+ pterm.Info.Println("No generator to destroy.")
+ } else {
pterm.Info.Println("Generator destroyed.")
- }
+ }📝 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.
| _, err = stack.Destroy(cmd.Context(), destroyOptions...) | |
| // Ugly check... | |
| if err != nil && !strings.Contains(err.Error(), "no resource named") { | |
| return err | |
| } | |
| if strings.Contains(err.Error(), "no resource named") { | |
| pterm.Info.Println("No generator to destroy.") | |
| } else { | |
| pterm.Info.Println("Generator destroyed.") | |
| _, err = stack.Destroy(cmd.Context(), destroyOptions...) | |
| // Ugly check... | |
| if err != nil { | |
| if !strings.Contains(err.Error(), "no resource named") { | |
| return err | |
| } | |
| // err is non-nil and indicates “no resource named” | |
| pterm.Info.Println("No generator to destroy.") | |
| } else { | |
| pterm.Info.Println("Generator destroyed.") | |
| } |
🤖 Prompt for AI Agents (early access)
In test/rolling-upgrades/cmd/root.go around lines 248 to 256, the code calls err.Error() without checking if err is nil, which can cause a nil-pointer panic when Destroy succeeds. Fix this by first checking if err is not nil before calling err.Error(), and restructure the conditional logic to avoid calling err.Error() when err is nil.
| optdestroy.Target([]string{ | ||
| "urn:pulumi:ledger-tests-rolling-upgrades::ledger::Formance:Ledger$Formance:Ledger:Tools:Generator::generator", | ||
| }), | ||
| optdestroy.TargetDependents(), | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Hard-coded URN contains a different stack name (“ledger-tests-rolling-upgrades”).
The CLI lets users change --stack-name, but this URN is fixed and even differs from the default tests-rolling-upgrades.
Targets will never match, making the generator impossible to delete.
Consider building the URN dynamically, e.g.:
stackURN := fmt.Sprintf("urn:pulumi:%s::ledger::Formance:Ledger$Formance:Ledger:Tools:Generator::generator", stack.Name())
optdestroy.Target([]string{stackURN}),🏁 Script executed:
#!/bin/bash
set -e
echo "Listing test/rolling-upgrades directory structure"
fd . test/rolling-upgrades
echo
echo "Locating root.go"
find test/rolling-upgrades -type f -name root.go
echo
echo "Showing code lines 1-150 of root.go"
sed -n '1,150p' test/rolling-upgrades/cmd/root.go || true
echo
echo "Showing lines 200-260 of root.go"
sed -n '200,260p' test/rolling-upgrades/cmd/root.go || true
echo
echo "Searching for 'stack.Name' usage"
rg -n "stack.Name" test/rolling-upgrades/cmd/root.go || true
echo
echo "Searching for stack variable declaration"
rg -n "stack *:=" test/rolling-upgrades/cmd/root.go || trueLength of output: 6383
Fix hard-coded URN to use the selected stack name
The destroyGeneratorIfExists function always tries to destroy a resource in the "ledger-tests-rolling-upgrades" stack, but the CLI flag defaults to "tests-rolling-upgrades" (or whatever is passed via --stack-name). This mismatch means the destroy target never matches and the generator will never be removed.
• File: test/rolling-upgrades/cmd/root.go
• Function: destroyGeneratorIfExists (around line 236)
Replace the hard-coded target:
optdestroy.Target([]string{
"urn:pulumi:ledger-tests-rolling-upgrades::ledger::Formance:Ledger$Formance:Ledger:Tools:Generator::generator",
}),With a dynamically built URN:
stackURN := fmt.Sprintf(
"urn:pulumi:%s::ledger::Formance:Ledger$Formance:Ledger:Tools:Generator::generator",
stack.Name(),
)
optdestroy.Target([]string{stackURN}),This ensures the destroy operation always uses the actual stack name provided at runtime.
🤖 Prompt for AI Agents (early access)
In test/rolling-upgrades/cmd/root.go around lines 236 to 240, the destroyGeneratorIfExists function uses a hard-coded URN with a fixed stack name "ledger-tests-rolling-upgrades" which does not match the dynamic stack name passed via CLI. Replace the hard-coded URN string with a dynamically constructed one using fmt.Sprintf and stack.Name() to build the URN, then pass this dynamic URN to optdestroy.Target. This ensures the destroy target matches the actual stack name at runtime.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #895 +/- ##
==========================================
- Coverage 82.18% 82.00% -0.19%
==========================================
Files 141 141
Lines 7613 7746 +133
==========================================
+ Hits 6257 6352 +95
- Misses 1039 1068 +29
- Partials 317 326 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.