feat(ledger): defaulting bucket and metadata #31
Conversation
WalkthroughRemoves per-resource logger fields and logger injections, adds OpenTelemetry tracing utilities and a ResourceTracer wrapper, makes ledger Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User (terraform)
participant T as Terraform CLI
participant P as Provider
participant R as ResourceTracer
participant S as Resource (e.g., Ledger)
participant A as Backend API
U->>T: terraform apply
T->>P: call Resource Create
P->>R: invoke traced resource
R->>R: start span (TraceError)
R->>S: call Create(ctx)
S->>A: API Create (may omit bucket → backend sets to "_default")
A-->>S: API Create response
S->>S: (optional) GetLedger to fetch real fields
S-->>R: return with diagnostics
R->>R: end span, record error/status
R-->>P: return to provider
P-->>T: apply result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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: 0
🧹 Nitpick comments (2)
.env.exemple (1)
21-21: Consider minor style improvements.Static analysis suggests a few optional refinements:
- Remove quotes around
-json(unnecessary for simple flags)- Add a blank line at the end of the file
- Move
TF_CLI_ARGSbeforeTF_LOGfor alphabetical orderingThese are stylistic preferences rather than functional issues.
-TF_CLI_ARGS="-json" + +TF_CLI_ARGS=-jsoninternal/resources/ledger_ledger.go (1)
133-135: Null check is now redundant but harmless.With the default value set in the schema,
plan.Bucketshould never be null—it will contain either the user-specified value or"_default". The null check will always pass, making this condition unnecessary.However, this doesn't cause any issues and can be considered defensive programming.
If you'd like to clean this up:
- if !plan.Bucket.IsNull() { - config.V2CreateLedgerRequest.Bucket = pointer.For(plan.Bucket.ValueString()) - } + config.V2CreateLedgerRequest.Bucket = pointer.For(plan.Bucket.ValueString())
📜 Review details
Configuration used: CodeRabbit UI
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 (3)
.env.exemple(1 hunks)internal/resources/ledger_ledger.go(2 hunks)tests/e2e/ledger_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/ledger_test.go (1)
tests/e2e/main_test.go (3)
CloudProvider(28-28)StackProvider(29-29)RegionName(30-30)
🪛 dotenv-linter (3.3.0)
.env.exemple
[warning] 21-21: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 21-21: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 21-21: [UnorderedKey] The TF_CLI_ARGS key should go before the TF_LOG key
(UnorderedKey)
🔇 Additional comments (4)
internal/resources/ledger_ledger.go (2)
17-17: LGTM! Default bucket implementation looks correct.The addition of
Computed: trueandDefault: stringdefault.StaticString("_default")correctly implements the default bucket behavior. When users don't specify a bucket, it will automatically be set to"_default".Also applies to: 58-60
58-60: Confirm_defaultis API’s expected default bucket
Ensure explicitly sending"_default"(instead of omitting the field) matches the API’s default bucket behavior.tests/e2e/ledger_test.go (2)
15-69: LGTM! Tests correctly verify default bucket behavior.The test restructuring and renaming to
TestLedgerDefaultappropriately reflects the new default bucket functionality. Key improvements:
- Parallel test execution with proper provider factory setup
- Terraform version checks
- Multiple steps verify idempotency (same config applied twice)
- RefreshState step ensures state refresh works correctly
- Bucket expectations correctly set to
"_default"when not explicitly specifiedThe test thoroughly validates that the default bucket value is applied and persisted correctly.
102-102: LGTM! Bucket expectations updated correctly.The state checks now correctly expect
bucketto be"_default"when not explicitly specified in the configuration, aligning with the schema changes ininternal/resources/ledger_ledger.go.Also applies to: 130-130
Dav-14
force-pushed
the
fix/ledger_default_bucket
branch
2 times, most recently
from
e9d374f to
ca0f82e
Compare
Dav-14
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.env.exemple (1)
21-21: Address linting issues.The static analysis tool flagged minor formatting issues with this line:
- Missing blank line at the end of the file
- Quote characters in the value (the
-jsonflag is wrapped in quotes)- Key ordering (TF_CLI_ARGS should appear before TF_LOG for consistency)
Apply this diff to address the issues:
-TF_CLI_ARGS="-json" +TF_CLI_ARGS=-json +And consider moving this line to appear before the
TF_LOGdeclarations (lines 18-20) for better organization.
📜 Review details
Configuration used: CodeRabbit UI
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 (3)
.env.exemple(1 hunks)internal/resources/ledger_ledger.go(3 hunks)tests/e2e/ledger_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/ledger_test.go (1)
tests/e2e/main_test.go (3)
CloudProvider(28-28)StackProvider(29-29)RegionName(30-30)
🪛 dotenv-linter (3.3.0)
.env.exemple
[warning] 21-21: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 21-21: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 21-21: [UnorderedKey] The TF_CLI_ARGS key should go before the TF_LOG key
(UnorderedKey)
⏰ 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). (1)
- GitHub Check: Tests (E2E)
🔇 Additional comments (6)
internal/resources/ledger_ledger.go (4)
17-18: LGTM! Imports added for default value helpers.The imports for
mapdefaultandstringdefaultare correctly added to support the new default values for the bucket and metadata attributes.
57-62: LGTM! Bucket now defaults to "_default".The bucket attribute is now Optional + Computed with a default value of "_default". This ensures users don't need to explicitly specify a bucket, and the resource will use "_default" when omitted.
68-74: LGTM! Metadata now defaults to an empty map.The metadata attribute is now Optional + Computed with a default empty map. This provides a consistent default when metadata is not specified.
136-138: Note: Behavioral change in bucket handling.With the bucket attribute now being Computed with a default value,
plan.Bucket.IsNull()will always return false (it will contain either the user-provided value or the default "_default"). This means the bucket will now always be sent to the API, whereas previously it might have been omitted when not specified by the user.Ensure this aligns with the API's expectations and doesn't cause any unexpected behavior.
tests/e2e/ledger_test.go (2)
15-68: LGTM! Test properly validates default bucket behavior.The renamed
TestLedgerDefaultfunction now includes:
- Proper parallel test setup with provider factories
- Two sequential configurations that create a ledger without specifying a bucket
- State checks that verify the bucket defaults to "_default"
- A refresh state step to ensure stability
This thoroughly validates the new default bucket functionality.
71-145: LGTM! Metadata test updated for default bucket.The
TestLedgerWithMetadatafunction correctly validates:
- Ledger creation with metadata but no explicit bucket specification
- State checks confirming the bucket defaults to "_default" (lines 102, 130)
- Metadata updates work correctly with the default bucket
- RefreshState ensures consistency
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
==========================================
- Coverage 56.94% 53.91% -3.03%
==========================================
Files 19 20 +1
Lines 1454 1519 +65
==========================================
- Hits 828 819 -9
- Misses 519 583 +64
- Partials 107 117 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Dav-14
force-pushed
the
fix/ledger_default_bucket
branch
from
ca0f82e to
83c6f67
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
Justfile (1)
56-70: Consider consolidating[group('terraform')]declarations.Each terraform target has its own
[group('terraform')]declaration (lines 56, 60, 64, 68). While valid, Justfile supports grouping multiple recipes under a single group attribute for better readability:-[group('terraform')] -init examples="install-verif": build - @cd examples/{{examples}} && terraform init -upgrade - -[group('terraform')] -plan examples="install-verif": build - @cd examples/{{examples}} && terraform plan -generate-config-out=generated.tf - -[group('terraform')] -apply examples="install-verif": build - @cd examples/{{examples}} && terraform apply -auto-approve - -[group('terraform')] -destroy examples="install-verif": build - @cd examples/{{examples}} && terraform destroy -auto-approve +[group('terraform')] +init examples="install-verif": build + @cd examples/{{examples}} && terraform init -upgrade + +plan examples="install-verif": build + @cd examples/{{examples}} && terraform plan -generate-config-out=generated.tf + +apply examples="install-verif": build + @cd examples/{{examples}} && terraform apply -auto-approve + +destroy examples="install-verif": build + @cd examples/{{examples}} && terraform destroy -auto-approvepkg/tracing/tracing.go (3)
10-24: Variable shadowing and redundant declarations.Lines 11, 16: The local variable
traceshadows the importedtracepackage (line 7), which could cause confusion. Consider renaming it tospan.Lines 14-16: The zero-value declarations for
kandvare redundant since line 16 reassigns them immediately.Apply this diff to address both issues:
-func TraceTuple[K, V any](ctx context.Context, tracer trace.Tracer, name string, fn func(ctx context.Context) (K, V, error), opts ...trace.SpanStartOption) (K, V, error) { - ctx, trace := tracer.Start(ctx, name, opts...) - defer trace.End() - - var k K - var v V - k, v, err := fn(ctx) +func TraceTuple[K, V any](ctx context.Context, tracer trace.Tracer, name string, fn func(ctx context.Context) (K, V, error), opts ...trace.SpanStartOption) (K, V, error) { + ctx, span := tracer.Start(ctx, name, opts...) + defer span.End() + + k, v, err := fn(ctx) if err != nil { - trace.RecordError(err) - trace.SetStatus(codes.Error, err.Error()) + span.RecordError(err) + span.SetStatus(codes.Error, err.Error()) return k, v, err } return k, v, nil }
26-39: Variable shadowing and unused zero value.Line 27: The local variable
traceshadows the importedtracepackage.Line 30:
zeroRetis only used in the error path, making its declaration on line 30 slightly redundant (though acceptable for clarity).Apply this diff to rename the span variable:
func Trace[RET any](ctx context.Context, tracer trace.Tracer, name string, fn func(ctx context.Context) (RET, error), opts ...trace.SpanStartOption) (RET, error) { - ctx, trace := tracer.Start(ctx, name, opts...) - defer trace.End() + ctx, span := tracer.Start(ctx, name, opts...) + defer span.End() var zeroRet RET ret, err := fn(ctx) if err != nil { - trace.RecordError(err) - trace.SetStatus(codes.Error, err.Error()) + span.RecordError(err) + span.SetStatus(codes.Error, err.Error()) return zeroRet, err } return ret, nil }
41-51: Variable shadowing.Line 42: The local variable
traceshadows the importedtracepackage.Apply this diff:
func TraceError(ctx context.Context, tracer trace.Tracer, name string, fn func(ctx context.Context) error, opts ...trace.SpanStartOption) error { - ctx, trace := tracer.Start(ctx, name, opts...) - defer trace.End() + ctx, span := tracer.Start(ctx, name, opts...) + defer span.End() if err := fn(ctx); err != nil { - trace.RecordError(err) - trace.SetStatus(codes.Error, err.Error()) + span.RecordError(err) + span.SetStatus(codes.Error, err.Error()) return err } return nil }internal/resources/reconciliation_policy.go (1)
238-239: TODO: Remove resource from state if not found.Lines 238-239 note a TODO to remove the resource from state if not found during Read, which would trigger recreation. This pattern should be applied consistently across all resources.
Do you want me to open a new issue to track this task across all resources, or would you like me to generate a script to identify all Read methods that need this pattern?
internal/resources/resource_tracer.go (2)
186-200: Missing debug logs in Schema operation.Lines 192-193: Unlike other operations (ValidateConfig, ImportState, Configure, Create, Delete, Read, Update), the Schema wrapper does not include debug log statements for "called" and "completed" events. This inconsistency could make debugging harder.
Apply this diff to add debug logging:
func (r *ResourceTracer) Schema(ctx context.Context, req resource.SchemaRequest, resp *resource.SchemaResponse) { ctx = logging.ContextWithLogger(ctx, r.logger) operation := "Schema" if v, ok := r.underlyingValue.(resource.Resource); ok { _ = tracing.TraceError(ctx, otlp.Tracer, operation, func(ctx context.Context) error { ctx = injectTraceContext(ctx, v, operation) + logging.FromContext(ctx).Debug(operation + " called") + defer logging.FromContext(ctx).Debug(operation + " completed") v.Schema(ctx, req, resp) if resp.Diagnostics.HasError() { return ErrSchema } return nil }) } }
202-216: Missing debug logs in Update operation.Lines 208-209: The Update wrapper does not include debug log statements for "called" and "completed" events, unlike other operations. This inconsistency could make debugging harder.
Apply this diff to add debug logging:
func (r *ResourceTracer) Update(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) { ctx = logging.ContextWithLogger(ctx, r.logger) operation := "Update" if v, ok := r.underlyingValue.(resource.Resource); ok { _ = tracing.TraceError(ctx, otlp.Tracer, operation, func(ctx context.Context) error { ctx = injectTraceContext(ctx, v, operation) + logging.FromContext(ctx).Debug(operation + " called") + defer logging.FromContext(ctx).Debug(operation + " completed") v.Update(ctx, req, resp) if resp.Diagnostics.HasError() { return ErrUpdate } return nil }) } }
📜 Review details
Configuration used: CodeRabbit UI
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 (14)
.env.exemple(1 hunks)Justfile(1 hunks)internal/otlp/tracer.go(1 hunks)internal/resources/ledger_ledger.go(6 hunks)internal/resources/noop.go(1 hunks)internal/resources/payments_connectors.go(2 hunks)internal/resources/payments_pool.go(2 hunks)internal/resources/reconciliation_policy.go(7 hunks)internal/resources/resource_tracer.go(1 hunks)internal/resources/webhook_config.go(2 hunks)internal/server/provider.go(3 hunks)pkg/tracing/tracing.go(1 hunks)tests/e2e/ledger_test.go(3 hunks)tests/integration/noop_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
internal/server/provider.go (7)
internal/resources/webhook_config.go (1)
NewWebhooks(37-41)internal/resources/ledger_ledger.go (1)
NewLedger(39-43)internal/resources/noop.go (1)
NewNoop(24-28)internal/resources/payments_connectors.go (1)
NewPaymentsConnectors(119-123)internal/resources/payments_pool.go (1)
NewPaymentsPool(33-37)internal/resources/reconciliation_policy.go (1)
NewReconciliationPolicy(69-73)internal/resources/resource_tracer.go (1)
NewResourceTracer(23-31)
internal/resources/ledger_ledger.go (2)
internal/stores.go (1)
ModuleStore(32-38)internal/server/sdk/errors.go (1)
HandleStackError(13-59)
internal/resources/payments_pool.go (1)
internal/stores.go (1)
ModuleStore(32-38)
tests/e2e/ledger_test.go (1)
tests/e2e/main_test.go (3)
CloudProvider(28-28)StackProvider(29-29)RegionName(30-30)
internal/resources/reconciliation_policy.go (1)
internal/stores.go (1)
ModuleStore(32-38)
pkg/tracing/tracing.go (1)
internal/otlp/tracer.go (1)
Tracer(7-7)
internal/resources/resource_tracer.go (2)
pkg/tracing/tracing.go (1)
TraceError(41-51)internal/otlp/tracer.go (1)
Tracer(7-7)
internal/resources/payments_connectors.go (1)
internal/stores.go (1)
ModuleStore(32-38)
internal/resources/webhook_config.go (1)
internal/stores.go (1)
ModuleStore(32-38)
🪛 dotenv-linter (3.3.0)
.env.exemple
[warning] 21-21: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 21-21: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 21-21: [UnorderedKey] The TF_CLI_ARGS key should go before the TF_LOG key
(UnorderedKey)
🔇 Additional comments (20)
Justfile (1)
56-58: LGTM! Newinittarget follows existing conventions.The addition of the
inittarget is well-implemented and consistent with the existing terraform targets. It correctly depends on thebuildtarget, uses the same default example value, and employs the-upgradeflag which is appropriate for development workflows.tests/integration/noop_test.go (1)
47-61: Verify intentional reduction in test coverage.Two test cases have been commented out: one for invalid
client_idand one for missingclient_secret. This reduces validation coverage.Were these scenarios intentionally removed? If so, please document why these validation paths are no longer required. If not, consider restoring them or relocating to a more appropriate test suite.
internal/resources/noop.go (1)
24-27: LGTM!The constructor signature has been simplified to remove the logger dependency, aligning with the broader architectural shift to use a tracing wrapper at the provider level instead of per-resource loggers.
tests/e2e/ledger_test.go (2)
15-69: LGTM!The new
TestLedgerDefaulttest properly validates the default bucket behavior. The test creates a ledger without specifying a bucket and verifies that it defaults to"_default", which aligns with the resource changes makingbucketcomputed with a default value.
102-102: LGTM!The test assertions have been correctly updated to expect
bucket="_default"instead of custom bucket values, which aligns with the new computed default behavior of the ledger resource.Also applies to: 130-130
internal/resources/payments_pool.go (1)
23-36: LGTM!The removal of the logger field and simplification of the constructor align with the architectural shift to use provider-level tracing wrappers instead of per-resource loggers.
internal/resources/webhook_config.go (1)
26-40: LGTM!The removal of logger dependencies is consistent with the architectural changes across all resources in this PR.
internal/resources/ledger_ledger.go (3)
27-42: LGTM!The removal of the logger field and simplification of the constructor are consistent with the architectural changes across all resources.
52-65: LGTM!Making
bucketcomputed and providing a default empty map formetadataimproves the user experience by allowing these fields to be optional while still tracking their actual values from the API.
157-173: Approve change: fetch ledger state with GetLedger
Confirm API returns expected defaults when inputs are omitted:
- Omitted
bucketyields the server default bucket (e.g._default)- Omitted
metadatayields an empty mapinternal/server/provider.go (2)
319-329: LGTM!The resources are now wrapped with
ResourceTracerto provide tracing capabilities at the provider level. This replaces the previous approach of passing loggers to individual resource constructors, centralizing observability concerns.
473-473: LGTM!Adding the
"provider": "stack"contextual field to the logger improves observability by making it clear which provider is generating log entries.internal/otlp/tracer.go (1)
1-7: LGTM!The shared tracer instance provides a centralized OpenTelemetry tracer for the provider. The tracer name
"github.com/formancehq/terraform-provider-stack"follows the convention of using the full package path.internal/resources/payments_connectors.go (1)
48-50: LGTM! Logger removal aligns with tracer-based observability.The removal of the logger field and the updated constructor signature are consistent with the broader refactoring where resources no longer manage their own loggers. Instead, the
ResourceTracerwrapper (introduced ininternal/resources/resource_tracer.go) injects logging context at runtime.Also applies to: 119-123
internal/resources/reconciliation_policy.go (4)
28-30: LGTM! Logger removal aligns with tracer-based observability.The removal of the logger field and the updated constructor are consistent with the broader refactoring where
ResourceTracerinjects logging context at runtime.Also applies to: 69-73
85-87: LGTM! PlanModifiers correctly enforce replacement.The
RequiresReplace()plan modifiers onledger_name,name,payments_pool_id, andledger_queryappropriately mark these fields as immutable, triggering resource replacement when they change.Also applies to: 92-94, 99-101, 106-108
261-261: LGTM! Appropriate warning for unimplemented Update.The warning clearly communicates that the Update method is not implemented and the resource will be recreated. This is appropriate given the
RequiresReplace()plan modifiers on all configurable fields.
152-152: Logger injection verified in ResourceTracer.ValidateConfig
logging.ContextWithLogger(ctx, r.logger)is invoked before callingv.ValidateConfig, so no changes are necessary.internal/resources/resource_tracer.go (2)
23-61: LGTM! Solid foundation for resource tracing.The constructor, struct, sentinel errors, and
injectTraceContexthelper establish a clean pattern for wrapping resources with tracing and logging. The use of OpenTelemetry propagation and contextual logging enrichment is appropriate.
64-79: LGTM! Consistent tracing wrapper pattern.The wrapper methods for ValidateConfig, ImportState, Configure, Create, Delete, and Read follow a consistent pattern:
- Inject logger into context
- Check if underlying resource implements the interface
- Wrap execution with tracing
- Inject trace context
- Log entry/exit
- Call underlying method
- Return sentinel error if diagnostics has errors
This provides excellent observability while keeping resources decoupled from logging concerns.
Also applies to: 81-97, 99-115, 117-133, 135-151, 168-184
| TF_LOG=TRACE | ||
| TF_LOG_FORMAT=JSON | ||
| TF_LOG_PATH=./terraform.log | ||
| TF_CLI_ARGS="-json" |
There was a problem hiding this comment.
Don’t set TF_CLI_ARGS to -json globally
TF_CLI_ARGS is prepended to every Terraform command. Most commands (terraform init, apply, etc.) reject -json, so sourcing this example will make the CLI fail with “flag provided but not defined: -json”. Use the command-scoped env vars instead (e.g., TF_CLI_ARGS_plan=-json) or drop this line.
-TF_CLI_ARGS="-json"
+# Example: enable JSON output only for plan
+# TF_CLI_ARGS_plan="-json"📝 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.
| TF_CLI_ARGS="-json" | |
| # Example: enable JSON output only for plan | |
| # TF_CLI_ARGS_plan="-json" |
🧰 Tools
🪛 dotenv-linter (3.3.0)
[warning] 21-21: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 21-21: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 21-21: [UnorderedKey] The TF_CLI_ARGS key should go before the TF_LOG key
(UnorderedKey)
🤖 Prompt for AI Agents
In .env.exemple around line 21, TF_CLI_ARGS is set globally to "-json", which
breaks most terraform commands; remove or comment out this global assignment and
either delete the line or replace it with command-scoped variables such as
TF_CLI_ARGS_plan=-json (or TF_CLI_ARGS_apply=-json) so only the intended
terraform subcommands receive the -json flag.
No description provided.