ESO-424:Added e2e test cases from openshift test private.#160
ESO-424:Added e2e test cases from openshift test private.#160siddhibhor-56 wants to merge 2 commits into
Conversation
WalkthroughAdds provider-focused e2e coverage for AWS synchronization, PushSecret, ownership, decoding, and generator workflows. Adds AWS and generator resource builders, AWS API helpers, namespace termination handling, and a govulncheck vulnerability filter update. ChangesProvider end-to-end coverage
Vulnerability filter update
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderE2ESuite
participant KubernetesAPI
participant ExternalSecretsOperator
participant AWSSecretsManager
participant AWSSSMParameterStore
ProviderE2ESuite->>KubernetesAPI: Create SecretStore and synchronization resources
ExternalSecretsOperator->>AWSSecretsManager: Read or write secret values
ExternalSecretsOperator->>AWSSSMParameterStore: Read or write parameters
ExternalSecretsOperator->>KubernetesAPI: Reconcile Kubernetes Secret
ProviderE2ESuite->>KubernetesAPI: Verify synchronized Secret
ProviderE2ESuite->>AWSSecretsManager: Verify secret round trip
ProviderE2ESuite->>AWSSSMParameterStore: Verify parameter round trip
Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": bin/kube-api-linter.so, plugin: not implemented Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: siddhibhor-56 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/e2e/helpers_test.go (1)
75-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the typed phase constant over the string literal.
Comparing
ns.Status.Phaseagainst the literal"Terminating"works but is fragile to typos and refactors. Use thecorev1.NamespaceTerminatingconstant fromk8s.io/api/core/v1.♻️ Proposed change
- if ns.Status.Phase != "Terminating" { + if ns.Status.Phase != corev1.NamespaceTerminating { return nil }Ensure
corev1 "k8s.io/api/core/v1"is imported.🤖 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 `@test/e2e/helpers_test.go` at line 75, The namespace phase check in the helper test is using a fragile string literal instead of the typed Kubernetes constant. Update the comparison in the namespace status assertion to use corev1.NamespaceTerminating from k8s.io/api/core/v1, and ensure the test file imports corev1 so the phase check stays consistent with the API types.test/e2e/provider_e2e_test.go (1)
273-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
Fail()instead ofExpect(false).To(BeTrue())for the missing-condition case.
g.Expect(false).To(BeTrue(), ...)works but is less idiomatic;Fail("expected Ready condition with ownership error")(orStopTrying) reads more clearly when the conditions loop finds noReadycondition.🤖 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 `@test/e2e/provider_e2e_test.go` around lines 273 - 289, The missing-condition fallback in the ownership-error check is using a non-idiomatic false expectation; update the `Eventually` block in `provider_e2e_test.go` so that the `Ready` condition search in the `conds` loop uses `Fail("expected Ready condition with ownership error")` (or `StopTrying`) when no matching condition is found, rather than `g.Expect(false).To(BeTrue(), ...)`.
🤖 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 `@test/e2e/provider_e2e_test.go`:
- Around line 91-101: Add meaningful failure messages to the suite-setup
assertions in provider_e2e_test.go so Ginkgo failures are easier to diagnose.
Update the key Expect(...).To(Succeed()) calls around namespace creation,
VerifyPodsReadyByPrefix, and ensureExternalSecretsConfigReady in the setup flow
to include short, specific messages. Apply the same pattern consistently to
other resource-creation and readiness checks in this file, using the surrounding
helpers and setup steps as anchors.
- Around line 592-593: The AWS credential copy step is discarding the result of
CopyAWSCredsToNamespace, which can hide setup failures and cause later
SecretStore/PushSecret checks to fail confusingly. Update the provider_e2e_test
flow to assert that CopyAWSCredsToNamespace succeeds, matching the earlier AWS
Parameter Store setup pattern, and keep the failure close to the credential-copy
step for clearer test diagnostics.
---
Nitpick comments:
In `@test/e2e/helpers_test.go`:
- Line 75: The namespace phase check in the helper test is using a fragile
string literal instead of the typed Kubernetes constant. Update the comparison
in the namespace status assertion to use corev1.NamespaceTerminating from
k8s.io/api/core/v1, and ensure the test file imports corev1 so the phase check
stays consistent with the API types.
In `@test/e2e/provider_e2e_test.go`:
- Around line 273-289: The missing-condition fallback in the ownership-error
check is using a non-idiomatic false expectation; update the `Eventually` block
in `provider_e2e_test.go` so that the `Ready` condition search in the `conds`
loop uses `Fail("expected Ready condition with ownership error")` (or
`StopTrying`) when no matching condition is found, rather than
`g.Expect(false).To(BeTrue(), ...)`.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 70d9ba79-663d-4969-821c-d2d4ca8627c2
⛔ Files ignored due to path filters (76)
test/go.sumis excluded by!**/*.sumvendor/github.com/aws/aws-sdk-go/service/ssm/api.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/service.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/waiters.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/AUTHORSis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/compression.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/conn.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/join.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/json.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/mask.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/mask_safe.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/prepared.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/proxy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/server.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/MAINTAINERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/NOTICEis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/connection.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/handlers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/priority.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/PATENTSis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/dictionary.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/read.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/write.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/stream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/flowrate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/io.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/util.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/internal/socks/client.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/internal/socks/socks.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/dial.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/direct.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/per_host.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/proxy.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/socks5.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy/connection.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy/upgrade.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/dial.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/transport.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/upgradeaware.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/third_party/forked/golang/netutil/addr.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/errorstream.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/fallback.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/reader.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/remotecommand.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/resize.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/spdy.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v1.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v2.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v3.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v4.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v5.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/websocket.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/transport/spdy/spdy.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/transport/websocket/roundtripper.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/util/exec/exec.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (5)
test/e2e/helpers_test.gotest/e2e/provider_e2e_test.gotest/go.modtest/utils/aws_provider_resources.gotest/utils/generator_resources.go
| got, err := providerClientset.CoreV1().Namespaces().Create(ctx, namespace, metav1.CreateOptions{}) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| providerTestNamespace = got.GetName() | ||
|
|
||
| By("Waiting for operator pod to be ready") | ||
| Expect(utils.VerifyPodsReadyByPrefix(ctx, providerClientset, operatorNamespace, []string{ | ||
| operatorPodPrefix, | ||
| })).To(Succeed()) | ||
|
|
||
| By("Ensuring ExternalSecretsConfig cluster CR exists and is Ready") | ||
| Expect(ensureExternalSecretsConfigReady(ctx)).To(Succeed()) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add failure messages to key suite-setup assertions.
Several setup Expect(...).To(Succeed()) calls (e.g., namespace creation, operator/operand readiness, ensureExternalSecretsConfigReady) lack messages, making BeforeAll/BeforeEach failures harder to triage. Adding short messages like "failed to create provider test namespace" improves diagnosability. This pattern recurs throughout the file (e.g., resource creation and readiness waits).
As per coding guidelines: "Flag Ginkgo test assertions that lack meaningful failure messages."
🤖 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 `@test/e2e/provider_e2e_test.go` around lines 91 - 101, Add meaningful failure
messages to the suite-setup assertions in provider_e2e_test.go so Ginkgo
failures are easier to diagnose. Update the key Expect(...).To(Succeed()) calls
around namespace creation, VerifyPodsReadyByPrefix, and
ensureExternalSecretsConfigReady in the setup flow to include short, specific
messages. Apply the same pattern consistently to other resource-creation and
readiness checks in this file, using the surrounding helpers and setup steps as
anchors.
Source: Coding guidelines
|
/retest |
|
/retest |
|
/test-all |
|
/test all |
siddhibhor-56
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@siddhibhor-56: This pull request references ESO-424 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/e2e/helpers_test.go`:
- Around line 68-76: Update the namespace termination check in the helper around
the namespace Get call to treat any non-nil ns.DeletionTimestamp as terminating,
regardless of ns.Status.Phase. Preserve the existing nil return for namespaces
that are neither marked for deletion nor in the Terminating phase.
In `@test/e2e/provider_e2e_test.go`:
- Around line 125-131: Label the AWS-dependent specs in the “AWS Parameter
Store” and “AWS Secrets Manager” contexts, including the [OCP-81813] push spec,
with [Skipped:Disconnected]. Preserve their existing AWS labels and test
behavior while ensuring all specs that call AWS SSM or Secrets Manager endpoints
are excluded in disconnected environments.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4eae5032-69ca-401f-ac67-2efbf9cb4922
⛔ Files ignored due to path filters (6)
vendor/github.com/aws/aws-sdk-go/service/ssm/api.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/service.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/waiters.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (4)
test/e2e/helpers_test.gotest/e2e/provider_e2e_test.gotest/utils/aws_provider_resources.gotest/utils/generator_resources.go
| ns, err := clientset.CoreV1().Namespaces().Get(ctx, namespace, metav1.GetOptions{}) | ||
| if k8serrors.IsNotFound(err) { | ||
| return nil | ||
| } | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if ns.Status.Phase != "Terminating" { | ||
| return nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,140p' test/e2e/helpers_test.go
printf '\n--- usages ---\n'
rg -n "Terminating|DeletionTimestamp|is.*terminating|namespace.*terminating|helpers_test.go" test/e2e -g '*.go'Repository: openshift/external-secrets-operator
Length of output: 5754
🏁 Script executed:
sed -n '2050,2110p' test/e2e/e2e_test.go
python3 - <<'PY'
from pathlib import Path
text = Path('test/e2e/helpers_test.go').read_text()
start = text.index('func waitForNamespaceTermination')
print(text[start:text.index('// resourceType defines', start)])
PYRepository: openshift/external-secrets-operator
Length of output: 3179
🏁 Script executed:
rg -n "ensureExternalSecretsConfigReady|waitForNamespaceTermination|OperandDefaultNamespace" test/e2e -g '*.go'
sed -n '1,90p' test/e2e/helpers_test.goRepository: openshift/external-secrets-operator
Length of output: 4188
🌐 Web query:
Kubernetes NamespaceStatus phase Terminating deletionTimestamp before phase changes namespace deletion semantics
💡 Result:
In Kubernetes, the interaction between the Namespace.Status.Phase and the metadata.deletionTimestamp is a core mechanism of the graceful deletion process [1][2]. When a DELETE request is issued for a namespace, the API server sets the metadata.deletionTimestamp to the current server time [1][3]. This timestamp acts as the primary indicator that a graceful deletion has been requested [3][4]. Upon observing this change, the namespace-controller updates the Namespace.Status.Phase from Active to Terminating [1][5]. The deletion process follows these semantics: 1. Request and Marking: The presence of a non-nil deletionTimestamp prevents the creation of new objects within the namespace [6]. The phase transition to Terminating signals to users and system components that the namespace is undergoing cleanup and is no longer available for new workloads [7][8]. 2. Resource Cleanup: The namespace-controller begins iterating through and deleting all content (resources) associated with the namespace [1][6]. 3. Finalization: The namespace itself contains a spec.finalizers list (typically including the kubernetes finalizer) [1][2]. The namespace cannot be removed from storage until this list is empty [3][4]. The namespace-controller is responsible for purging the namespace's contents and, upon successful cleanup, removing the finalizers [1][6]. 4. Permanent Removal: Once all finalizers are removed and the deletionTimestamp is set, the namespace-controller sends a final DELETE action to the API server to permanently remove the namespace object from etcd storage [1][2]. If a namespace remains stuck in the Terminating phase, it is typically because one or more finalizers have not been removed, often due to a controller failing to successfully clean up the associated resources or a manual error (e.g., specifying a non-existent finalizer) [9][3][8]. Because the deletionTimestamp is already set, the namespace is effectively "locked" into the deletion flow until the finalizers are cleared [3][4]. Top results: [1][5][6][9][3]
Citations:
- 1: https://github.com/kubernetes/design-proposals-archive/blob/main/architecture/namespaces.md
- 2: https://github.com/kubernetes/kubernetes/blob/release-1.2/docs/design/namespaces.md
- 3: https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers/
- 4: https://kubernetes.io/docs/reference/kubernetes-api/definitions/object-meta-v1-meta/
- 5: https://github.com/kubernetes/kubernetes/blob/v1.23.4/pkg/controller/namespace/deletion/namespaced_resources_deleter.go
- 6: https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/namespace/namespace_controller.go
- 7: https://kubernetes.io/docs/reference/kubernetes-api/cluster-resources/namespace-v1/
- 8: https://kubernetes.io/docs/tasks/administer-cluster/namespaces/
- 9: https://www.redhat.com/en/blog/troubleshooting-terminating-namespaces
Treat a non-nil deletionTimestamp as terminating. Lines 75-76 can return while the namespace is already marked for deletion but still reports Active, letting setup continue before teardown finishes and making later readiness checks flaky.
🤖 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 `@test/e2e/helpers_test.go` around lines 68 - 76, Update the namespace
termination check in the helper around the namespace Get call to treat any
non-nil ns.DeletionTimestamp as terminating, regardless of ns.Status.Phase.
Preserve the existing nil return for namespaces that are neither marked for
deletion nor in the Terminating phase.
| Context("AWS Parameter Store", Label("Platform:AWS"), func() { | ||
| const awsCredsLocalName = "aws-creds" | ||
|
|
||
| BeforeAll(func() { | ||
| By("Copying AWS credentials into the test namespace") | ||
| Expect(utils.CopyAWSCredsToNamespace(ctx, providerClientset, providerTestNamespace, awsCredsLocalName)).To(Succeed()) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
AWS provider specs reach external AWS endpoints; add a disconnected guard.
The AWS Parameter Store / Secrets Manager contexts (and the [OCP-81813] push spec at Line 579) drive live calls to AWS SSM and Secrets Manager APIs, which are unreachable in disconnected/IPv6-only environments. Consider labeling these AWS-dependent specs with [Skipped:Disconnected] so they don't fail there.
As per coding guidelines: "flag tests that require connectivity to public internet hosts ... Tests failing in IPv6-only disconnected environments should be flagged unless already protected with [Skipped:Disconnected] label."
🤖 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 `@test/e2e/provider_e2e_test.go` around lines 125 - 131, Label the
AWS-dependent specs in the “AWS Parameter Store” and “AWS Secrets Manager”
contexts, including the [OCP-81813] push spec, with [Skipped:Disconnected].
Preserve their existing AWS labels and test behavior while ensuring all specs
that call AWS SSM or Secrets Manager endpoints are excluded in disconnected
environments.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e/provider_e2e_test.go`:
- Line 78: Update the provider setup around NewDynamicResourceLoader to pass the
active Ginkgo test handle instead of constructing &testing.T{}; ensure the
loader’s t.Logf and require calls use that real test context.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: be6c882d-b25a-49df-ac55-384fb15ba621
📒 Files selected for processing (5)
hack/govulncheck.shtest/e2e/helpers_test.gotest/e2e/provider_e2e_test.gotest/utils/aws_provider_resources.gotest/utils/generator_resources.go
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/govulncheck.sh
- test/e2e/helpers_test.go
| providerClientset = suiteClientset | ||
| providerDynamicClient = suiteDynamicClient | ||
| providerRuntimeClient = suiteRuntimeClient | ||
| providerLoader = utils.NewDynamicResourceLoader(ctx, &testing.T{}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect NewDynamicResourceLoader and DeleteFromUnstructured usage of *testing.T
fd -e go -t f . test/utils | xargs rg -nP 'func NewDynamicResourceLoader|DeleteFromUnstructured|CreateFromUnstructured'
ast-grep run --pattern '$T.Fatal($$$)' --lang go test/utils
ast-grep run --pattern '$T.Fatalf($$$)' --lang go test/utilsRepository: openshift/external-secrets-operator
Length of output: 1372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- test/e2e/provider_e2e_test.go around line 78 ---'
sed -n '60,90p' test/e2e/provider_e2e_test.go | cat -n
echo
echo '--- test/utils/dynamic_resources.go around loader methods ---'
sed -n '1,220p' test/utils/dynamic_resources.go | cat -nRepository: openshift/external-secrets-operator
Length of output: 10138
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate NewClientsConfigForTest ---'
rg -n "func NewClientsConfigForTest|NewClientsConfigForTest\(" test -g '*.go'
echo
echo '--- inspect the implementation ---'
file=$(rg -l "func NewClientsConfigForTest" test -g '*.go' | head -n 1)
sed -n '1,220p' "$file" | cat -n
echo
echo '--- search for zero-value testing.T patterns ---'
rg -n "&testing\.T\{\}|testing\.T\{\}" -g '*.go' .Repository: openshift/external-secrets-operator
Length of output: 9412
Pass a real test handle to NewDynamicResourceLoader test/e2e/provider_e2e_test.go:78
DynamicResourceLoader uses t.Logf, require.NoError, and require.NotEmpty, so &testing.T{} is not a safe stand-in here. Use the active Ginkgo test handle or refactor the loader to return errors instead of depending on *testing.T.
🤖 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 `@test/e2e/provider_e2e_test.go` at line 78, Update the provider setup around
NewDynamicResourceLoader to pass the active Ginkgo test handle instead of
constructing &testing.T{}; ensure the loader’s t.Logf and require calls use that
real test context.
|
@siddhibhor-56: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Added test cases for :-
Summary by CodeRabbit
e2eprovider suite for AWS Parameter Store and Secrets Manager, validating secret sync, updates, decoding (Auto), adoption/ownership behavior, conflict handling, and PushSecret round-trips.