test: improve critical deployment coverage#451
Open
flemzord wants to merge 2 commits into
Open
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThis PR adds comprehensive unit test coverage for the operator codebase. It introduces a new test utility module ( ChangesTest Coverage and Infrastructure
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
flemzord
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
internal/resources/jobs/job_test.go (2)
138-154: 💤 Low valueGood error propagation test.
The test correctly verifies that PreCreate errors prevent job creation. Consider using a more specific error check for the job Get operation to confirm it's a NotFound error rather than some other error.
📝 Optional enhancement
+import apierrors "k8s.io/apimachinery/pkg/api/errors" + job := &batchv1.Job{} err = ctx.GetClient().Get(context.Background(), types.NamespacedName{Namespace: "stack0", Name: "uid-123-migrate"}, job) -require.Error(t, err) +require.True(t, apierrors.IsNotFound(err), "expected NotFound error, got: %v", err)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/resources/jobs/job_test.go` around lines 138 - 154, Update the assertion in TestHandleRunsPreCreateErrorBeforeCreatingJob to explicitly check that the failed Get is a Kubernetes NotFound error instead of a generic failure: after calling ctx.GetClient().Get(...) for the Job (namespaced name "stack0"/"uid-123-migrate"), use the k8s API errors helper (apierrors.IsNotFound(err) or errors.IsNotFound equivalent) to assert the Job does not exist, keeping the rest of the test and the use of Handle and PreCreate the same.
62-62: 💤 Low valueConsider verifying the actual PodFailurePolicy content.
The test checks that
PodFailurePolicyis non-nil but doesn't verify the actual rules, actions, or exit codes that were configured (lines 28-34). For more thorough coverage, consider asserting the policy content matches the expected configuration.📝 Suggested enhancement
require.NotNil(t, job.Spec.PodFailurePolicy) +require.Len(t, job.Spec.PodFailurePolicy.Rules, 1) +require.Equal(t, batchv1.PodFailurePolicyActionFailJob, job.Spec.PodFailurePolicy.Rules[0].Action) +require.Equal(t, batchv1.PodFailurePolicyOnExitCodesOpIn, job.Spec.PodFailurePolicy.Rules[0].OnExitCodes.Operator) +require.Equal(t, []int32{1}, job.Spec.PodFailurePolicy.Rules[0].OnExitCodes.Values)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/resources/jobs/job_test.go` at line 62, Extend the test that currently only asserts require.NotNil(t, job.Spec.PodFailurePolicy) to validate the actual policy rules: inspect job.Spec.PodFailurePolicy.Rules and add assertions that the expected number of rules exist and each rule's fields match the expected configuration (e.g., rule.Action equals the expected action, rule.OnExitCodes.Operator and rule.OnExitCodes.Values contain the expected codes, and any expected OnPodConditions are present). Use the symbols job.Spec.PodFailurePolicy, job.Spec.PodFailurePolicy.Rules, and individual rule fields (Action, OnExitCodes.Operator, OnExitCodes.Values, OnPodConditions) to locate and assert the exact values.internal/testutil/context.go (1)
168-186: ⚡ Quick winAvoid duplicating
SplitKeywordWithDotlogic.The
splitKeywordWithDotfunction duplicates logic that already exists in the settings package assettingspkg.SplitKeywordWithDot(referenced ininternal/resources/ledgers/deployments_test.go:228). Consider importing and using the settings package function instead to avoid maintenance burden.♻️ Suggested fix
Import the settings package and use its function:
+ "github.com/formancehq/operator/v3/internal/resources/settings" func withSettingsIndexes(builder *fake.ClientBuilder) { builder.WithIndex(&v1beta1.Settings{}, "stack", func(obj client.Object) []string { settings := obj.(*v1beta1.Settings) return settings.Spec.Stacks }) builder.WithIndex(&v1beta1.Settings{}, "keylen", func(obj client.Object) []string { - settings := obj.(*v1beta1.Settings) - return []string{strconv.Itoa(len(splitKeywordWithDot(settings.Spec.Key)))} + s := obj.(*v1beta1.Settings) + return []string{strconv.Itoa(len(settings.SplitKeywordWithDot(s.Spec.Key)))} }) } - -func splitKeywordWithDot(key string) []string { - segments := "" - inQuote := false - for _, item := range key { - switch item { - case '"': - inQuote = !inQuote - case '.': - if !inQuote { - segments += " " - continue - } - segments += string(item) - default: - segments += string(item) - } - } - return strings.Split(segments, " ") -}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/testutil/context.go` around lines 168 - 186, Replace the local duplicate function splitKeywordWithDot by removing it and calling the existing settingspkg.SplitKeywordWithDot instead; import the settings package (as settingspkg or the project's usual alias) and update any callers in this file to use settingspkg.SplitKeywordWithDot, then delete the local splitKeywordWithDot implementation to avoid duplicate logic and maintenance overhead.internal/resources/ledgers/deployments_test.go (1)
218-274: ⚖️ Poor tradeoffConsider consolidating index setup with
testutil.NewContext.The
newLedgerContextWithDependencyIndexesfunction duplicates index setup that already exists intestutil.NewContext:
- Settings indexes (lines 224-230) are already in
testutil.NewContext(line 158-166 in context.go)- Unstructured stack indexes for Auth/Gateway (lines 245-246) are already in
testutil.NewContext(lines 76-77 in context.go)The BrokerTopic indexes (lines 231-244) appear to be the only additional indexes needed. Consider either:
- Adding the BrokerTopic indexes to
testutil.NewContextso all tests can use them- Extending
testutil.NewContextto accept optional index configurationThis would reduce duplication and make test setup more consistent across the codebase.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/resources/ledgers/deployments_test.go` around lines 218 - 274, The helper newLedgerContextWithDependencyIndexes duplicates index registration already present in testutil.NewContext (Settings indexes and unstructured stack indexes); update testutil.NewContext instead to include the BrokerTopic indexes or allow passing additional index registrations so newLedgerContextWithDependencyIndexes can delegate to testutil.NewContext without re-registering indexes; specifically either add the two builder.WithIndex(...) calls for v1beta1.BrokerTopic (the "stack" and ".spec.service" index functions) into testutil.NewContext, or change testutil.NewContext to accept a slice of index registration closures and invoke them (so withLedgerUnstructuredStackIndex and existing Settings indexes are not duplicated in newLedgerContextWithDependencyIndexes).api/formance.com/v1beta1/status_methods_test.go (2)
81-81: 💤 Low valueClarify the intent of unchecked method calls.
The calls to
isEventPublisher()at lines 81, 89, 97, and 126 discard the return value. If the intent is to verify the method can be invoked without panicking, consider adding a comment or assigning to_to make this explicit. If there's an expected boolean value, assert it instead.📝 Example clarification approaches
Option 1 - Assign to underscore to show intentional discard:
- ledger.isEventPublisher() + _ = ledger.isEventPublisher()Option 2 - Add explanatory comment:
+ // Verify isEventPublisher can be invoked ledger.isEventPublisher()Also applies to: 89-89, 97-97, 126-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/formance.com/v1beta1/status_methods_test.go` at line 81, The call to isEventPublisher() on the ledger variable currently discards the return value; make the intent explicit by either assigning the result to the blank identifier (e.g., _ = ledger.isEventPublisher()) if you only need to ensure it doesn't panic, or add an explanatory comment above the call stating "intentional discard — checking for panic only", or if a particular boolean is expected, replace the bare call with an assertion (e.g., require.True(t, ledger.isEventPublisher()) or equivalent) to validate the result; apply this change to all occurrences of ledger.isEventPublisher() in the test.
153-153: 💤 Low valueClarify the intent of unchecked
isResource()calls.Similar to the
isEventPublisher()calls,topic.isResource()anddatabase.isResource()discard their return values. Consider the same clarification approaches: assign to_, add a comment, or assert the expected value if deterministic.Also applies to: 156-156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/formance.com/v1beta1/status_methods_test.go` at line 153, The calls topic.isResource() and database.isResource() are invoked with their return values discarded, which obscures intent; update those sites (topic.isResource() and database.isResource()) to make intent explicit by either assigning the result to the blank identifier (e.g., _ = topic.isResource()), adding a short comment explaining why the return value is ignored, or replace with an assertion/require (e.g., require.True(t, topic.isResource())) when the value is deterministic so tests document the expected outcome.internal/resources/brokers/reconcile_test.go (1)
74-82: ⚡ Quick winAssert that skip-path does not create a Job.
This test currently only checks
NoError. It can still pass if a regression creates a provisioning Job. Please add a NotFound assertion forbroker-uid-create-streamto lock in the “skip” behavior.Suggested patch
import ( "context" "fmt" "testing" "github.com/stretchr/testify/require" batchv1 "k8s.io/api/batch/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ func TestCreateOneStreamByStackSkipsReadyBroker(t *testing.T) { t.Parallel() stack := &v1beta1.Stack{ObjectMeta: testutil.ObjectMeta("stack0")} broker := brokerFixture(v1beta1.ModeOneStreamByStack) broker.Status.Ready = true - require.NoError(t, createOneStreamByStack(testutil.NewContext(stack, broker), stack, broker, testutil.MustParseURI("nats://nats.stack0:4222"))) + ctx := testutil.NewContext(stack, broker) + require.NoError(t, createOneStreamByStack(ctx, stack, broker, testutil.MustParseURI("nats://nats.stack0:4222"))) + + err := ctx.GetClient().Get(ctx, types.NamespacedName{Name: "broker-uid-create-stream", Namespace: "stack0"}, &batchv1.Job{}) + require.True(t, apierrors.IsNotFound(err)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/resources/brokers/reconcile_test.go` around lines 74 - 82, Update TestCreateOneStreamByStackSkipsReadyBroker to also assert that no provisioning Job was created: after calling createOneStreamByStack(stack, broker, ...), use the test context's Kubernetes client to try to GET a batchv1.Job named "<broker.UID>-create-stream" (or fmt.Sprintf("%s-create-stream", broker.UID)) in the stack/broker namespace and assert the GET returns a NotFound error (use k8s.io/apimachinery/pkg/api/errors.IsNotFound or the test helper equivalent). This ensures the skip path in createOneStreamByStack did not create the Job.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/resources/gatewayhttpapis/create_test.go`:
- Around line 18-19: The test assertions are inverted: change the checks so
RuleSecured().Secured is asserted true and RuleUnsecured().Secured is asserted
false; locate the assertions referencing RuleSecured and RuleUnsecured in
create_test.go and replace require.False(t, RuleSecured().Secured) with a
require.True check for RuleSecured, and replace require.True(t,
RuleUnsecured().Secured) with a require.False check for RuleUnsecured so they
match the function names and usage elsewhere.
In `@internal/resources/gateways/dns_settings_test.go`:
- Around line 7-11: The test currently uses a generic require.Error after the
delete reconciliation which can mask unrelated client failures; update the
assertion to explicitly check for a Kubernetes NotFound error by using
apierrors.IsNotFound(err) (import "k8s.io/apimachinery/pkg/api/errors" as
apierrors) when checking the result of the Get call used to verify deletion (the
err variable from the client.Get in the delete test), e.g. replace
require.Error(t, err) with require.True(t, apierrors.IsNotFound(err)) so the
test verifies the resource was actually removed.
In `@internal/resources/settings/opentelemetry_test.go`:
- Around line 39-47: The test currently assumes POD_NAME was found and
dereferences podName.ValueFrom.FieldRef.FieldPath which can nil-panic; update
the assertion sequence after the loop to first assert the env contained POD_NAME
(e.g., require.NotNil(t, podName.ValueFrom) / require.NotNil(t,
podName.ValueFrom.FieldRef) or require.True/require.NotEmpty on podName) before
asserting podName.ValueFrom.FieldRef.FieldPath equals "metadata.name" so that
podName, ValueFrom and FieldRef are all validated prior to dereference
(references: variable podName, slice env, env var name "POD_NAME", fields
ValueFrom, FieldRef, FieldPath, and test helper require).
---
Nitpick comments:
In `@api/formance.com/v1beta1/status_methods_test.go`:
- Line 81: The call to isEventPublisher() on the ledger variable currently
discards the return value; make the intent explicit by either assigning the
result to the blank identifier (e.g., _ = ledger.isEventPublisher()) if you only
need to ensure it doesn't panic, or add an explanatory comment above the call
stating "intentional discard — checking for panic only", or if a particular
boolean is expected, replace the bare call with an assertion (e.g.,
require.True(t, ledger.isEventPublisher()) or equivalent) to validate the
result; apply this change to all occurrences of ledger.isEventPublisher() in the
test.
- Line 153: The calls topic.isResource() and database.isResource() are invoked
with their return values discarded, which obscures intent; update those sites
(topic.isResource() and database.isResource()) to make intent explicit by either
assigning the result to the blank identifier (e.g., _ = topic.isResource()),
adding a short comment explaining why the return value is ignored, or replace
with an assertion/require (e.g., require.True(t, topic.isResource())) when the
value is deterministic so tests document the expected outcome.
In `@internal/resources/brokers/reconcile_test.go`:
- Around line 74-82: Update TestCreateOneStreamByStackSkipsReadyBroker to also
assert that no provisioning Job was created: after calling
createOneStreamByStack(stack, broker, ...), use the test context's Kubernetes
client to try to GET a batchv1.Job named "<broker.UID>-create-stream" (or
fmt.Sprintf("%s-create-stream", broker.UID)) in the stack/broker namespace and
assert the GET returns a NotFound error (use
k8s.io/apimachinery/pkg/api/errors.IsNotFound or the test helper equivalent).
This ensures the skip path in createOneStreamByStack did not create the Job.
In `@internal/resources/jobs/job_test.go`:
- Around line 138-154: Update the assertion in
TestHandleRunsPreCreateErrorBeforeCreatingJob to explicitly check that the
failed Get is a Kubernetes NotFound error instead of a generic failure: after
calling ctx.GetClient().Get(...) for the Job (namespaced name
"stack0"/"uid-123-migrate"), use the k8s API errors helper
(apierrors.IsNotFound(err) or errors.IsNotFound equivalent) to assert the Job
does not exist, keeping the rest of the test and the use of Handle and PreCreate
the same.
- Line 62: Extend the test that currently only asserts require.NotNil(t,
job.Spec.PodFailurePolicy) to validate the actual policy rules: inspect
job.Spec.PodFailurePolicy.Rules and add assertions that the expected number of
rules exist and each rule's fields match the expected configuration (e.g.,
rule.Action equals the expected action, rule.OnExitCodes.Operator and
rule.OnExitCodes.Values contain the expected codes, and any expected
OnPodConditions are present). Use the symbols job.Spec.PodFailurePolicy,
job.Spec.PodFailurePolicy.Rules, and individual rule fields (Action,
OnExitCodes.Operator, OnExitCodes.Values, OnPodConditions) to locate and assert
the exact values.
In `@internal/resources/ledgers/deployments_test.go`:
- Around line 218-274: The helper newLedgerContextWithDependencyIndexes
duplicates index registration already present in testutil.NewContext (Settings
indexes and unstructured stack indexes); update testutil.NewContext instead to
include the BrokerTopic indexes or allow passing additional index registrations
so newLedgerContextWithDependencyIndexes can delegate to testutil.NewContext
without re-registering indexes; specifically either add the two
builder.WithIndex(...) calls for v1beta1.BrokerTopic (the "stack" and
".spec.service" index functions) into testutil.NewContext, or change
testutil.NewContext to accept a slice of index registration closures and invoke
them (so withLedgerUnstructuredStackIndex and existing Settings indexes are not
duplicated in newLedgerContextWithDependencyIndexes).
In `@internal/testutil/context.go`:
- Around line 168-186: Replace the local duplicate function splitKeywordWithDot
by removing it and calling the existing settingspkg.SplitKeywordWithDot instead;
import the settings package (as settingspkg or the project's usual alias) and
update any callers in this file to use settingspkg.SplitKeywordWithDot, then
delete the local splitKeywordWithDot implementation to avoid duplicate logic and
maintenance overhead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3faef755-4e43-404a-8b81-661be1740509
⛔ Files ignored due to path filters (1)
pkg/client/formance.com/v1beta1/client_test.gois excluded by!pkg/client/**
📒 Files selected for processing (29)
api/formance.com/v1beta1/shared_test.goapi/formance.com/v1beta1/status_methods_test.gointernal/core/dependencies_test.gointernal/core/utils_test.gointernal/resources/applications/application_settings_test.gointernal/resources/auths/env_test.gointernal/resources/brokerconsumers/controller_test.gointernal/resources/brokers/reconcile_test.gointernal/resources/brokers/utils_test.gointernal/resources/caddy/caddy_test.gointernal/resources/databases/postgres_env_test.gointernal/resources/gatewayhttpapis/create_test.gointernal/resources/gateways/configuration_settings_test.gointernal/resources/gateways/dns_settings_test.gointernal/resources/gateways/gateways_test.gointernal/resources/gateways/ingress_settings_test.gointernal/resources/jobs/job_test.gointernal/resources/ledgers/deployments_test.gointernal/resources/payments/deployments.gointernal/resources/payments/deployments_test.gointernal/resources/payments/finalizer_settings_test.gointernal/resources/registries/registries_test.gointernal/resources/services/services_settings_test.gointernal/resources/settings/getters_test.gointernal/resources/settings/helpers_test.gointernal/resources/settings/opentelemetry_test.gointernal/resources/stacks/init_test.gointernal/resources/webhooks/deployment_test.gointernal/testutil/context.go
Comment on lines
+18
to
+19
| require.False(t, RuleSecured().Secured) | ||
| require.True(t, RuleUnsecured().Secured) |
Contributor
There was a problem hiding this comment.
Fix backwards test assertions.
The test assertions are inverted:
RuleSecured()should return a rule withSecured = true(a secured rule)RuleUnsecured()should return a rule withSecured = false(an unsecured rule)
But the test expects the opposite. This contradicts the function naming and the usage pattern on lines 22 and 40 where Secured: true is used for secured rules.
🐛 Proposed fix
- require.False(t, RuleSecured().Secured)
- require.True(t, RuleUnsecured().Secured)
+ require.True(t, RuleSecured().Secured)
+ require.False(t, RuleUnsecured().Secured)📝 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.
Suggested change
| require.False(t, RuleSecured().Secured) | |
| require.True(t, RuleUnsecured().Secured) | |
| require.True(t, RuleSecured().Secured) | |
| require.False(t, RuleUnsecured().Secured) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/resources/gatewayhttpapis/create_test.go` around lines 18 - 19, The
test assertions are inverted: change the checks so RuleSecured().Secured is
asserted true and RuleUnsecured().Secured is asserted false; locate the
assertions referencing RuleSecured and RuleUnsecured in create_test.go and
replace require.False(t, RuleSecured().Secured) with a require.True check for
RuleSecured, and replace require.True(t, RuleUnsecured().Secured) with a
require.False check for RuleUnsecured so they match the function names and usage
elsewhere.
Comment on lines
+7
to
+11
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| externaldnsv1alpha1 "sigs.k8s.io/external-dns/apis/v1alpha1" | ||
| "sigs.k8s.io/external-dns/endpoint" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/resources/gateways/dns_settings_test.go | head -80 | tail -20Repository: formancehq/operator
Length of output: 1047
🏁 Script executed:
cat -n internal/resources/gateways/dns_settings_test.go | head -20Repository: formancehq/operator
Length of output: 774
🏁 Script executed:
# Check the context of this test - look for the test function name and what it's testing
sed -n '54,77p' internal/resources/gateways/dns_settings_test.goRepository: formancehq/operator
Length of output: 1170
Assert NotFound explicitly after delete reconciliation.
At lines 73–76, require.Error is too broad; it can pass on unrelated client failures. Assert Kubernetes NotFound to verify the delete actually succeeded rather than failing for other reasons (network, permissions, etc.).
Proposed fix
import (
"testing"
"github.com/stretchr/testify/require"
+ apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"
@@
err := ctx.GetClient().Get(ctx, types.NamespacedName{Name: "gateway-private", Namespace: "stack0"}, &externaldnsv1alpha1.DNSEndpoint{})
-require.Error(t, err)
+require.Error(t, err)
+require.True(t, apierrors.IsNotFound(err))
err = ctx.GetClient().Get(ctx, types.NamespacedName{Name: "gateway-public", Namespace: "stack0"}, &externaldnsv1alpha1.DNSEndpoint{})
-require.Error(t, err)
+require.Error(t, err)
+require.True(t, apierrors.IsNotFound(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.
Suggested change
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |
| "k8s.io/apimachinery/pkg/types" | |
| "sigs.k8s.io/controller-runtime/pkg/client" | |
| externaldnsv1alpha1 "sigs.k8s.io/external-dns/apis/v1alpha1" | |
| "sigs.k8s.io/external-dns/endpoint" | |
| "github.com/stretchr/testify/require" | |
| apierrors "k8s.io/apimachinery/pkg/api/errors" | |
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |
| "k8s.io/apimachinery/pkg/types" | |
| "sigs.k8s.io/controller-runtime/pkg/client" | |
| externaldnsv1alpha1 "sigs.k8s.io/external-dns/apis/v1alpha1" | |
| "sigs.k8s.io/external-dns/endpoint" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/resources/gateways/dns_settings_test.go` around lines 7 - 11, The
test currently uses a generic require.Error after the delete reconciliation
which can mask unrelated client failures; update the assertion to explicitly
check for a Kubernetes NotFound error by using apierrors.IsNotFound(err) (import
"k8s.io/apimachinery/pkg/api/errors" as apierrors) when checking the result of
the Get call used to verify deletion (the err variable from the client.Get in
the delete test), e.g. replace require.Error(t, err) with require.True(t,
apierrors.IsNotFound(err)) so the test verifies the resource was actually
removed.
Comment on lines
+39
to
+47
| var podName corev1.EnvVar | ||
| for _, item := range env { | ||
| if item.Name == "POD_NAME" { | ||
| podName = item | ||
| break | ||
| } | ||
| } | ||
| require.Equal(t, "metadata.name", podName.ValueFrom.FieldRef.FieldPath) | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the file to understand the context
find . -name "opentelemetry_test.go" -path "*/settings/*" | head -5Repository: formancehq/operator
Length of output: 115
🏁 Script executed:
# Also check if we can determine the Go version and dependencies to understand the types
git ls-files | grep -E "(go.mod|go.sum)" | head -3Repository: formancehq/operator
Length of output: 105
🏁 Script executed:
# Read the opentelemetry_test.go file to see the actual code
cat -n ./internal/resources/settings/opentelemetry_test.go | head -60Repository: formancehq/operator
Length of output: 2735
🏁 Script executed:
# Also check the go.mod to understand the Kubernetes dependencies
cat ./go.mod | grep -E "(kubernetes|corev1|k8s)" | head -10Repository: formancehq/operator
Length of output: 447
🏁 Script executed:
# Let's find and examine the GetOTELEnvVars function to understand what env vars it returns
find . -name "*.go" -path "*/settings/*" ! -name "*_test.go" | head -5Repository: formancehq/operator
Length of output: 289
🏁 Script executed:
# Search for GetOTELEnvVars function definition
rg "func GetOTELEnvVars" --type goRepository: formancehq/operator
Length of output: 225
🏁 Script executed:
# Read the GetOTELEnvVars function to understand what env vars it returns
cat -n ./internal/resources/settings/opentelemetry.go | head -100Repository: formancehq/operator
Length of output: 3697
🏁 Script executed:
# Let's continue reading opentelemetry.go to see the complete otelEnvVars function
cat -n ./internal/resources/settings/opentelemetry.go | sed -n '100,150p'Repository: formancehq/operator
Length of output: 730
🏁 Script executed:
# Check if there are any other code paths that might return env vars without POD_NAME
rg "POD_NAME" --type go ./internal/resources/settings/Repository: formancehq/operator
Length of output: 652
🏁 Script executed:
# Let's verify that POD_NAME is always added to the returned env vars
# Check all possible code paths in otelEnvVars
cat -n ./internal/resources/settings/opentelemetry.go | sed -n '50,120p'Repository: formancehq/operator
Length of output: 2961
🏁 Script executed:
# Check if there's any code that might skip adding POD_NAME or return early
rg "return.*nil.*nil" ./internal/resources/settings/opentelemetry.go -A 2 -B 2Repository: formancehq/operator
Length of output: 106
Guard against nil dereference when asserting POD_NAME.
At line 46, the test dereferences podName.ValueFrom.FieldRef without verifying that POD_NAME was found in the env vars. If a regression causes the env vars to be empty or to exclude POD_NAME, this will panic with a nil pointer dereference, obscuring the actual failure. Add an assertion to confirm POD_NAME is present and its fields are non-nil.
Proposed fix
var podName corev1.EnvVar
+foundPodName := false
for _, item := range env {
if item.Name == "POD_NAME" {
podName = item
+ foundPodName = true
break
}
}
+require.True(t, foundPodName, "POD_NAME env var should be present")
+require.NotNil(t, podName.ValueFrom)
+require.NotNil(t, podName.ValueFrom.FieldRef)
require.Equal(t, "metadata.name", podName.ValueFrom.FieldRef.FieldPath)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/resources/settings/opentelemetry_test.go` around lines 39 - 47, The
test currently assumes POD_NAME was found and dereferences
podName.ValueFrom.FieldRef.FieldPath which can nil-panic; update the assertion
sequence after the loop to first assert the env contained POD_NAME (e.g.,
require.NotNil(t, podName.ValueFrom) / require.NotNil(t,
podName.ValueFrom.FieldRef) or require.True/require.NotEmpty on podName) before
asserting podName.ValueFrom.FieldRef.FieldPath equals "metadata.name" so that
podName, ValueFrom and FieldRef are all validated prior to dereference
(references: variable podName, slice env, env var name "POD_NAME", fields
ValueFrom, FieldRef, FieldPath, and test helper require).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Coverage