feat: add a valueFrom field to Settings #347
Conversation
Signed-off-by: Sylvain Rabot <sylvain@formance.com>
WalkthroughAdds ValueFrom to SettingsSpec to allow values sourced from Secrets or ConfigMaps, makes Value optional, updates deep-copy methods, adds resolution logic with namespace fallback and optional handling, and updates controller-gen version. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant findMatchingSettings
participant resolveValueFrom
participant K8sClient
participant StackNS
participant FormanceNS
Caller->>findMatchingSettings: Request setting (ctx, stack, key)
alt Setting has inline Value
findMatchingSettings-->>Caller: return Value
else Setting has ValueFrom
findMatchingSettings->>resolveValueFrom: resolve (ctx, stackNS, valueFrom)
resolveValueFrom->>K8sClient: Get resource (Secret/ConfigMap) in StackNS
alt resource found
K8sClient-->>resolveValueFrom: data
resolveValueFrom-->>findMatchingSettings: return value
else resource not found (and required)
resolveValueFrom->>K8sClient: Get resource in FormanceNS (fallback)
K8sClient-->>resolveValueFrom: data or error
resolveValueFrom-->>findMatchingSettings: return value or error
else resource optional and not found
resolveValueFrom-->>findMatchingSettings: indicate optional-not-found
end
findMatchingSettings-->>Caller: resolved value or error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. 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: 1
📜 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 ignored due to path filters (24)
config/crd/bases/formance.com_authclients.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_auths.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_benthos.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_benthosstreams.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_brokerconsumers.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_brokers.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_brokertopics.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_databases.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_gatewayhttpapis.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_gateways.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_ledgers.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_orchestrations.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_payments.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_reconciliations.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_resourcereferences.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_searches.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_settings.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_stacks.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_stargates.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_versions.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_wallets.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_webhooks.yamlis excluded by!**/*.yamlconfig/rbac/role.yamlis excluded by!**/*.yamlgo.modis excluded by!**/*.mod
📒 Files selected for processing (5)
Justfile(1 hunks)api/formance.com/v1beta1/settings_types.go(2 hunks)api/formance.com/v1beta1/zz_generated.deepcopy.go(2 hunks)internal/resources/settings/helpers.go(4 hunks)internal/resources/settings/helpers_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/resources/settings/helpers_test.go (4)
internal/core/context.go (2)
NewContext(41-46)Context(10-16)api/formance.com/v1beta1/settings_types.go (3)
Settings(272-277)SettingsSpec(25-39)ValueFrom(42-49)internal/core/platform.go (1)
Platform(3-13)internal/resources/settings/helpers.go (1)
SplitKeywordWithDot(518-537)
api/formance.com/v1beta1/zz_generated.deepcopy.go (1)
api/formance.com/v1beta1/settings_types.go (1)
ValueFrom(42-49)
internal/resources/settings/helpers.go (2)
internal/core/context.go (1)
Context(10-16)api/formance.com/v1beta1/settings_types.go (3)
SettingsList(290-294)Settings(272-277)ValueFrom(42-49)
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // Build list of objects for fake client | ||
| objects := make([]client.Object, 0) | ||
| for _, secret := range tt.secrets { | ||
| objects = append(objects, secret) | ||
| } | ||
| for _, cm := range tt.configMaps { | ||
| objects = append(objects, cm) | ||
| } | ||
|
|
||
| fakeClient := fake.NewClientBuilder(). | ||
| WithScheme(scheme). | ||
| WithObjects(objects...). | ||
| Build() | ||
|
|
||
| // Create a mock manager | ||
| mockMgr := &mockManager{ | ||
| client: fakeClient, | ||
| scheme: scheme, | ||
| } | ||
|
|
||
| coreMgr := core.NewDefaultManager(mockMgr, core.Platform{ | ||
| Region: "test", | ||
| Environment: "test", | ||
| }) | ||
|
|
||
| ctx := core.NewContext(coreMgr, context.Background()) | ||
|
|
||
| value, err := findMatchingSettings(ctx, tt.stack, []v1beta1.Settings{tt.setting}, SplitKeywordWithDot(tt.setting.Spec.Key)...) | ||
|
|
||
| if tt.expectedError != "" { | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), tt.expectedError) | ||
| require.Nil(t, value) | ||
| } else { | ||
| require.NoError(t, err) | ||
| require.NotNil(t, value) | ||
| require.Equal(t, tt.expectedResult, *value) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Fix loop variable capture in parallel subtests.
Each closure shares the same loop variable tt; with t.Parallel() every subtest runs after the loop completes and only sees the final test case. Rebind tt inside the loop so each subtest operates on its own data.
- for _, tt := range tests {
- t.Run(tt.name, func(t *testing.T) {
+ for _, tt := range tests {
+ tt := tt
+ t.Run(tt.name, func(t *testing.T) {
t.Parallel()📝 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.
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| t.Parallel() | |
| // Build list of objects for fake client | |
| objects := make([]client.Object, 0) | |
| for _, secret := range tt.secrets { | |
| objects = append(objects, secret) | |
| } | |
| for _, cm := range tt.configMaps { | |
| objects = append(objects, cm) | |
| } | |
| fakeClient := fake.NewClientBuilder(). | |
| WithScheme(scheme). | |
| WithObjects(objects...). | |
| Build() | |
| // Create a mock manager | |
| mockMgr := &mockManager{ | |
| client: fakeClient, | |
| scheme: scheme, | |
| } | |
| coreMgr := core.NewDefaultManager(mockMgr, core.Platform{ | |
| Region: "test", | |
| Environment: "test", | |
| }) | |
| ctx := core.NewContext(coreMgr, context.Background()) | |
| value, err := findMatchingSettings(ctx, tt.stack, []v1beta1.Settings{tt.setting}, SplitKeywordWithDot(tt.setting.Spec.Key)...) | |
| if tt.expectedError != "" { | |
| require.Error(t, err) | |
| require.Contains(t, err.Error(), tt.expectedError) | |
| require.Nil(t, value) | |
| } else { | |
| require.NoError(t, err) | |
| require.NotNil(t, value) | |
| require.Equal(t, tt.expectedResult, *value) | |
| } | |
| }) | |
| for _, tt := range tests { | |
| tt := tt | |
| t.Run(tt.name, func(t *testing.T) { | |
| t.Parallel() | |
| // Build list of objects for fake client | |
| objects := make([]client.Object, 0) | |
| for _, secret := range tt.secrets { | |
| objects = append(objects, secret) | |
| } | |
| for _, cm := range tt.configMaps { | |
| objects = append(objects, cm) | |
| } | |
| fakeClient := fake.NewClientBuilder(). | |
| WithScheme(scheme). | |
| WithObjects(objects...). | |
| Build() | |
| // Create a mock manager | |
| mockMgr := &mockManager{ | |
| client: fakeClient, | |
| scheme: scheme, | |
| } | |
| coreMgr := core.NewDefaultManager(mockMgr, core.Platform{ | |
| Region: "test", | |
| Environment: "test", | |
| }) | |
| ctx := core.NewContext(coreMgr, context.Background()) | |
| value, err := findMatchingSettings(ctx, tt.stack, []v1beta1.Settings{tt.setting}, SplitKeywordWithDot(tt.setting.Spec.Key)...) | |
| if tt.expectedError != "" { | |
| require.Error(t, err) | |
| require.Contains(t, err.Error(), tt.expectedError) | |
| require.Nil(t, value) | |
| } else { | |
| require.NoError(t, err) | |
| require.NotNil(t, value) | |
| require.Equal(t, tt.expectedResult, *value) | |
| } | |
| }) | |
| } |
🤖 Prompt for AI Agents
internal/resources/settings/helpers_test.go around lines 528 to 570: the
subtests call t.Parallel() while closing over the loop variable tt, causing all
subtests to share the same tt value; rebind the loop variable at the start of
each iteration (e.g., local := tt) and use that local variable inside the t.Run
closure so each parallel subtest captures its own copy of the test case and test
data.
| } | ||
| if setting.Spec.ValueFrom != nil { | ||
| // Resolve value from secret or configmap | ||
| value, err := resolveValueFrom(ctx, stack, setting.Spec.ValueFrom) |
There was a problem hiding this comment.
Not sure referencing a secret from a stack namespace is a good idea.
You know, chicken and eggs problems.
But, I guess, the operator will eventually reconcile at the end.
| if err != nil { | ||
| if errors.Is(err, ErrResourceNotFound) || errors.Is(err, ErrOptionalResourceNotFound) { | ||
| // Try fallback to formance-system namespace | ||
| value, err2 := resolveValueFrom(ctx, "formance-system", setting.Spec.ValueFrom) |
There was a problem hiding this comment.
Maybe you can get the current namespace of the running pod? By injecting it using the downward api.
- Fix bug where optional resource check used wrong error variable (err vs err2) - Simplify code since CEL validation ensures exactly one of value/valueFrom is set - Remove test case for impossible state (both value and valueFrom set)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @internal/resources/settings/helpers.go:
- Line 33: The error message returned from the helper that wraps the underlying
error currently says "listings settings" which is a typo; update the wrapped
error string to "listing settings" (i.e., change fmt.Errorf("listings settings:
%w", err) to fmt.Errorf("listing settings: %w", err)) so the returned error
reads correctly; locate this change in the helper in
internal/resources/settings/helpers.go where that fmt.Errorf is used.
- Line 41: Replace the typo in the error string in the fmt.Errorf call that
currently reads fmt.Errorf("listings settings: %w", err) so it matches the
correct wording used elsewhere (e.g., change "listings settings" to "listing
settings" or the project's canonical phrase); update the string inside the
fmt.Errorf call accordingly where that return nil, fmt.Errorf(...) occurs.
🧹 Nitpick comments (2)
internal/resources/settings/helpers_test.go (1)
349-368: Consider adding test for optional key missing in existing secret.The test covers optional secret not found, but doesn't test the case where the secret exists but the key is missing with
Optional: true. Based on the implementation inhelpers.go(lines 457-459), this should also returnErrOptionalResourceNotFound. Consider adding a test case for this scenario.internal/resources/settings/helpers.go (1)
402-424: Consider defensive handling for optional resources in fallback scenario.When an optional resource is not found in the stack namespace, the fallback to
formance-systemshould also treat it as optional. Currently, the code only returns an empty string iferr2isErrOptionalResourceNotFound(line 415-418). However, since the samevalueFromobject is passed to bothresolveValueFromcalls, if the primary call returnedErrOptionalResourceNotFound, the fallback should inherit that "optional" context regardless of the error type returned by the second call.Consider checking whether the primary error indicated an optional resource:
if errors.Is(err, ErrOptionalResourceNotFound) || errors.Is(err2, ErrOptionalResourceNotFound) { empty := "" return &empty, nil }This makes the intent explicit and defensive against unexpected error type variations.
📜 Review details
Configuration used: Organization 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 (2)
internal/resources/settings/helpers.gointernal/resources/settings/helpers_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/resources/settings/helpers.go (4)
internal/resources/settings/settings.go (1)
New(25-36)internal/core/context.go (1)
Context(10-16)api/formance.com/v1beta1/settings_types.go (3)
SettingsList(290-294)Settings(272-277)ValueFrom(42-49)internal/tests/internal/bootstrap.go (1)
List(219-221)
internal/resources/settings/helpers_test.go (6)
internal/core/context.go (2)
NewContext(41-46)Context(10-16)api/formance.com/v1beta1/settings_types.go (3)
Settings(272-277)SettingsSpec(25-39)ValueFrom(42-49)api/formance.com/v1beta1/groupversion_info.go (1)
AddToScheme(44-44)api/formance.com/v1beta1/shared.go (1)
Object(294-300)internal/core/platform.go (1)
Platform(3-13)internal/resources/settings/helpers.go (1)
SplitKeywordWithDot(515-534)
🔇 Additional comments (6)
internal/resources/settings/helpers_test.go (3)
3-28: LGTM on imports.The new imports are appropriate for the fake client testing setup with controller-runtime.
210-224: LGTM on context passing.Passing
nilfor the manager is acceptable here since these tests only exercise the key-matching logic with directValuesettings, notValueFromresolution.
546-580: Mock implementation is adequate for test purposes.The
mockManagercorrectly implements the Manager interface for controller-runtime v0.17.1. The no-op implementations for unused methods, includingSetFields, are appropriate for testing.internal/resources/settings/helpers.go (3)
22-25: LGTM on sentinel errors.Well-defined sentinel errors enable clean error type checking with
errors.Is()for the fallback logic.
433-500: Well-structured ValueFrom resolution logic.The implementation correctly handles:
- Secret and ConfigMap lookups with proper namespace handling
- Optional resource and key scenarios
- ConfigMap
BinaryDatafallback- Descriptive error messages with namespace context
393-393: All call sites offindMatchingSettingshave been correctly updated.The function signature change to include
ctx core.Contextandstack stringparameters has been fully implemented across all three call sites:
- Internal call (line 44): Properly passes both
ctxandstackparameters- Test calls (lines 210, 531): Both pass appropriately constructed or derived
ctxandstackvalues
| "keylen": fmt.Sprint(len(keys)), | ||
| }); err != nil { | ||
| return nil, errors.Wrap(err, "listings settings") | ||
| return nil, fmt.Errorf("listings settings: %w", err) |
There was a problem hiding this comment.
Minor typo in error message.
"listings settings" should be "listing settings".
📝 Proposed fix
- return nil, fmt.Errorf("listings settings: %w", err)
+ return nil, fmt.Errorf("listing settings: %w", 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.
| return nil, fmt.Errorf("listings settings: %w", err) | |
| return nil, fmt.Errorf("listing settings: %w", err) |
🤖 Prompt for AI Agents
In @internal/resources/settings/helpers.go at line 33, The error message
returned from the helper that wraps the underlying error currently says
"listings settings" which is a typo; update the wrapped error string to "listing
settings" (i.e., change fmt.Errorf("listings settings: %w", err) to
fmt.Errorf("listing settings: %w", err)) so the returned error reads correctly;
locate this change in the helper in internal/resources/settings/helpers.go where
that fmt.Errorf is used.
| "keylen": fmt.Sprint(len(keys)), | ||
| }); err != nil { | ||
| return nil, errors.Wrap(err, "listings settings") | ||
| return nil, fmt.Errorf("listings settings: %w", err) |
There was a problem hiding this comment.
Same typo in second error message.
📝 Proposed fix
- return nil, fmt.Errorf("listings settings: %w", err)
+ return nil, fmt.Errorf("listing settings: %w", 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.
| return nil, fmt.Errorf("listings settings: %w", err) | |
| return nil, fmt.Errorf("listing settings: %w", err) |
🤖 Prompt for AI Agents
In @internal/resources/settings/helpers.go at line 41, Replace the typo in the
error string in the fmt.Errorf call that currently reads fmt.Errorf("listings
settings: %w", err) so it matches the correct wording used elsewhere (e.g.,
change "listings settings" to "listing settings" or the project's canonical
phrase); update the string inside the fmt.Errorf call accordingly where that
return nil, fmt.Errorf(...) occurs.
No description provided.