Conversation
|
/hold |
WalkthroughThis PR adds comprehensive AWS-focused end-to-end tests to the External Secrets Operator. It introduces new test scenarios covering basic operations, advanced features (secret refresh, templates, JSONPath extraction), and multiple secret store configurations. Support utilities for AWS secret management are added to the test infrastructure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
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.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": bin/kube-api-linter.so, plugin: not implemented Comment |
openshift-ci
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: emmajiafan 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/e2e_test.go (1)
146-164: Redundant AWS secret deletion - bothAfterAllanddeferattempt cleanup.The
AfterAllblock at lines 146-150 deletesawsSecretName, but the test at line 161-164 also has adeferthat deletes the same secret. This could cause the second deletion to fail with a "secret not found" error (though it's usingNotTo(HaveOccurred())which would fail the test).🔎 Proposed fix - remove duplicate cleanup
Consider removing the
AfterAllcleanup since the test already handles its own cleanup viadefer:- AfterAll(func() { - By("Deleting the AWS secret") - Expect(utils.DeleteAWSSecret(ctx, clientset, awsSecretName, awsSecretRegionName)). - NotTo(HaveOccurred(), "failed to delete AWS secret test/e2e") - })Or keep
AfterAllas a safety net but make the defer error tolerant.
🧹 Nitpick comments (7)
test/e2e/testdata/aws_invalid_secret_store.yaml (1)
13-13: Consider parameterizing the region for consistency.The region is hardcoded to
ap-south-1while other test manifests use${AWS_REGION}. For consistency and flexibility across test environments, consider using:- region: ap-south-1 + region: ${AWS_REGION}This allows testing invalid credential scenarios across different regions.
test/utils/conditions.go (2)
150-204: Consider extracting session creation to reduce duplication.The AWS session creation logic is duplicated across
CreateAWSSecret,UpdateAWSSecret,DeleteAWSSecret,CreateAWSParameter, andDeleteAWSParameter. This could be refactored into a helper function.🔎 Proposed refactor to extract session helper
+func newAWSSession(ctx context.Context, k8sClient *kubernetes.Clientset, region string) (*session.Session, error) { + id, key, err := fetchAWSCreds(ctx, k8sClient) + if err != nil { + return nil, err + } + + sess, err := session.NewSession(&aws.Config{ + Credentials: awscred.NewCredentials(&awscred.StaticProvider{Value: awscred.Value{ + AccessKeyID: id, + SecretAccessKey: key, + }}), + Region: aws.String(region), + }) + if err != nil { + return nil, fmt.Errorf("failed to create AWS session: %w", err) + } + return sess, nil +} func CreateAWSSecret(ctx context.Context, k8sClient *kubernetes.Clientset, secretName, secretValue, region string) error { - id, key, err := fetchAWSCreds(ctx, k8sClient) - if err != nil { - return err - } - - sess, err := session.NewSession(&aws.Config{ - Credentials: awscred.NewCredentials(&awscred.StaticProvider{Value: awscred.Value{ - AccessKeyID: id, - SecretAccessKey: key, - }}), - Region: aws.String(region), - }) + sess, err := newAWSSession(ctx, k8sClient, region) if err != nil { - return fmt.Errorf("failed to create AWS session: %w", err) + return err } // ... rest of functionAlso applies to: 263-291
280-286: SSM parameter creation will fail if parameter already exists.With
Overwrite: aws.Bool(false),CreateAWSParameterwill fail if the parameter already exists from a previous test run that didn't clean up properly. Consider usingOverwrite: aws.Bool(true)for test resilience, or document this behavior.test/e2e/testdata/aws_ssm_external_secret.yaml (1)
11-13: Minor inconsistency:kindis hardcoded while other manifests use placeholders.This manifest hardcodes
kind: ClusterSecretStorewhile other ExternalSecret manifests use${SECRETSTORE_KIND}. If SSM tests should only run against ClusterSecretStore, this is fine. Otherwise, consider parameterizing for consistency.test/e2e/scripts/run-tests.sh (1)
16-19: Unused variableYELLOW.The
YELLOWvariable is defined but never used in the script. Consider removing it or utilizing it for warning messages.test/e2e/scripts/verify-setup.sh (1)
36-36: Deprecated--shortflag for kubectl version.The
--shortflag is deprecated in newer kubectl versions. Consider usingkubectl version --client=false -o yamlor removing the version output entirely since cluster connectivity is already verified.🔎 Proposed fix
- kubectl version --short 2>/dev/null | grep "Server Version" || true + kubectl version -o yaml 2>/dev/null | grep -A1 "serverVersion:" | head -2 || truetest/e2e/e2e_test.go (1)
594-595: Silently ignoringjson.Marshalerror.The error from
json.Marshalis discarded. While this is unlikely to fail for a simple map, it's better practice to handle errors explicitly in tests to catch unexpected issues during test setup.🔎 Proposed fix
- secretBytes, _ := json.Marshal(nestedSecret) + secretBytes, err := json.Marshal(nestedSecret) + Expect(err).NotTo(HaveOccurred(), "failed to marshal nested secret") secretValue := string(secretBytes)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (279)
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/go.opentelemetry.io/auto/sdk/internal/telemetry/id.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/auto/sdk/internal/telemetry/number.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/auto/sdk/internal/telemetry/span.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/auto/sdk/internal/telemetry/status.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/auto/sdk/internal/telemetry/traces.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/auto/sdk/internal/telemetry/value.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/auto/sdk/span.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/auto/sdk/tracer.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/config.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/internal/parse.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/metadata_supplier.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/semconv.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/version.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/.codespellignoreis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/.golangci.ymlis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/.lycheeignoreis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/CODEOWNERSis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/CONTRIBUTING.mdis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/Makefileis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/README.mdis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/SECURITY-INSIGHTS.ymlis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/encoder.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/filter.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/internal/attribute.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/iterator.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/key.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/kv.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/set.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/value.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/baggage/baggage.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/codes/codes.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/dependencies.Dockerfileis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/internal/global/internal_logging.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/internal/global/trace.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/metric/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/propagation/baggage.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/propagation/propagation.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/propagation/trace_context.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/internal/x/x.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/builtin.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/container.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/env.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/host_id.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/os.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/os_release_unix.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/process.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/resource/resource.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/batch_span_processor.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/doc.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/id_generator.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/internal/x/README.mdis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/internal/x/x.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/provider.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/sampling.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/simple_span_processor.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/snapshot.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/span.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/tracer.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/version.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/version.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/internal/http.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.26.0/README.mdis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.26.0/attribute_group.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.26.0/metric.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.30.0/MIGRATION.mdis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.30.0/README.mdis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.30.0/attribute_group.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.30.0/doc.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.30.0/exception.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.30.0/metric.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.30.0/schema.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.34.0/attribute_group.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.34.0/error_type.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/MIGRATION.mdis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/README.mdis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/attribute_group.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/doc.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/error_type.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/exception.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/otelconv/metric.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/schema.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/auto.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/config.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/hex.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/attr.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/id.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/value.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/noop.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/noop/noop.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/trace.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/tracestate.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/version.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/versions.yamlis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/mod/modfile/read.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/mod/module/module.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/mod/semver/semver.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/context/context.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/frame.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/transport.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc7540.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc9218.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/trace/events.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/oauth2/deviceauth.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/oauth2/oauth2.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/oauth2/pkce.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/oauth2/token.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/oauth2/transport.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sync/errgroup/errgroup.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/cpu/cpu_arm64.sis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkerrors.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_386.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_amd64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_arm.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_arm64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_loong64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mips.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mips64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mips64le.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mipsle.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_ppc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_ppc64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_ppc64le.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_riscv64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_s390x.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_sparc64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsyscall_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_netbsd_arm.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/syscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/types_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/zsyscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/telemetry/codereview.cfgis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/term/terminal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/japanese/eucjp.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/japanese/iso2022jp.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/japanese/shiftjis.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/korean/euckr.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/simplifiedchinese/gbk.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/simplifiedchinese/hzgb2312.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/traditionalchinese/big5.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/unicode/unicode.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/time/rate/rate.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/diagnostic.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/appends/appends.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/asmdecl/asmdecl.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/assign/assign.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/atomic/atomic.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/bools/bools.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/buildssa/buildssa.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/buildtag/buildtag.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/cgocall/cgocall.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/copylock/copylock.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/ctrlflow/ctrlflow.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/deepequalerrors/deepequalerrors.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/defers/defers.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/directive/directive.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/errorsas/errorsas.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/fieldalignment/fieldalignment.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/framepointer/framepointer.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/hostport/hostport.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/httpmux/httpmux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/ifaceassert/ifaceassert.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/inspect/inspect.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/internal/analysisutil/util.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/internal/ctrlflowinternal/ctrlflowinternal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/loopclosure/loopclosure.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/lostcancel/lostcancel.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/any.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/bloop.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/doc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/errorsastype.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/fmtappendf.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/forvar.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/maps.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/minmax.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/modernize.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/newexpr.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/omitzero.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/plusbuild.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/rangeint.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/reflect.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/slices.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/slicescontains.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/slicesdelete.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/sortslice.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/stditerators.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/stringsbuilder.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/stringscut.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/stringscutprefix.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/stringsseq.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/testingcontext.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/waitgroup.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/nilfunc/nilfunc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/nilness/nilness.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/printf/printf.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/printf/types.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/reflectvaluecompare/reflectvaluecompare.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/shadow/shadow.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/sigchanyzer/sigchanyzer.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/slog/slog.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/stdmethods/stdmethods.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/stringintconv/string.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/testinggoroutine/testinggoroutine.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/testinggoroutine/util.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/tests/tests.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/timeformat/timeformat.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/unmarshal/unmarshal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/unreachable/unreachable.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/unsafeptr/unsafeptr.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/unusedresult/unusedresult.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/unusedwrite/unusedwrite.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/waitgroup/waitgroup.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ast/inspector/cursor.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/callgraph/vta/propagation.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/cfg/builder.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/cfg/cfg.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/packages/visit.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ssa/builder.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ssa/create.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ssa/emit.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ssa/func.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ssa/instantiate.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ssa/ssa.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ssa/ssautil/visit.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ssa/subst.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ssa/util.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/types/objectpath/objectpath.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/types/typeutil/map.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/imports/forward.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/analysis/analyzerutil/doc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/analysis/analyzerutil/extractdoc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/analysis/analyzerutil/readfile.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/analysis/analyzerutil/version.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/analysis/typeindex/typeindex.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/analysisinternal/analysis.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/analysisinternal/generated/generated.gois excluded by!**/generated/**,!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/astutil/stringlit.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/astutil/util.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/cfginternal/cfginternal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/gcimporter/bimport.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/gcimporter/iexport.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/gcimporter/iimport.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/goplsexport/export.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/packagepath/packagepath.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/refactor/delete.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/refactor/imports.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/refactor/refactor.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/ssainternal/ssainternal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/stdlib/deps.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/stdlib/import.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/stdlib/manifest.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typeparams/normalize.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typesinternal/element.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typesinternal/fx.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typesinternal/isnamed.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typesinternal/qualifier.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typesinternal/varkind.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typesinternal/varkind_go124.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typesinternal/zerovalue.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/versions/features.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/CONTRIBUTING.mdis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/balancer/pickfirst/pickfirst.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/balancer/pickfirst/pickfirstleaf/pickfirstleaf.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/balancer/roundrobin/roundrobin.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (21)
go.modtest/e2e/README.mdtest/e2e/e2e_test.gotest/e2e/scripts/README.mdtest/e2e/scripts/cleanup-credentials.shtest/e2e/scripts/run-tests.shtest/e2e/scripts/setup-aws-credentials.shtest/e2e/scripts/verify-setup.shtest/e2e/testdata/aws_external_secret_binary.yamltest/e2e/testdata/aws_external_secret_datafrom.yamltest/e2e/testdata/aws_external_secret_jsonpath.yamltest/e2e/testdata/aws_external_secret_multi_key.yamltest/e2e/testdata/aws_external_secret_refresh.yamltest/e2e/testdata/aws_external_secret_template.yamltest/e2e/testdata/aws_invalid_secret_store.yamltest/e2e/testdata/aws_k8s_secret_multi_key.yamltest/e2e/testdata/aws_secret_store.yamltest/e2e/testdata/aws_secret_store_namespace.yamltest/e2e/testdata/aws_ssm_external_secret.yamltest/e2e/testdata/aws_ssm_secret_store.yamltest/utils/conditions.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/scripts/README.mdtest/e2e/testdata/aws_external_secret_template.yamltest/e2e/testdata/aws_external_secret_binary.yamltest/e2e/testdata/aws_external_secret_jsonpath.yamltest/e2e/testdata/aws_ssm_secret_store.yamltest/e2e/scripts/setup-aws-credentials.shtest/e2e/testdata/aws_invalid_secret_store.yamltest/e2e/scripts/run-tests.shtest/e2e/testdata/aws_ssm_external_secret.yamltest/e2e/scripts/cleanup-credentials.shtest/e2e/README.mdtest/e2e/scripts/verify-setup.shtest/e2e/testdata/aws_external_secret_datafrom.yamltest/e2e/testdata/aws_external_secret_refresh.yamltest/e2e/testdata/aws_k8s_secret_multi_key.yamltest/utils/conditions.gotest/e2e/testdata/aws_secret_store_namespace.yamltest/e2e/testdata/aws_external_secret_multi_key.yamltest/e2e/testdata/aws_secret_store.yamlgo.modtest/e2e/e2e_test.go
🧬 Code graph analysis (1)
test/e2e/e2e_test.go (2)
test/utils/conditions.go (5)
ReplacePatternInAsset(250-261)GetRandomString(240-248)CreateAWSSecret(150-176)WaitForESOResourceReady(101-138)UpdateAWSSecret(178-204)api/v1alpha1/groupversion_info.go (1)
Resource(40-42)
🪛 LanguageTool
test/e2e/scripts/README.md
[style] ~210-~210: ‘without warning’ might be wordy. Consider a shorter alternative.
Context: ...pts will overwrite existing credentials without warning - Use cleanup-credentials.sh to remov...
(EN_WORDINESS_PREMIUM_WITHOUT_WARNING)
test/e2e/README.md
[grammar] ~90-~90: Use a hyphen to join words.
Context: ...scoped SecretStore** - Tests non-cluster scoped SecretStore 2. **Multiple data ke...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
test/e2e/scripts/README.md
172-172: Bare URL used
(MD034, no-bare-urls)
🪛 Shellcheck (0.11.0)
test/e2e/scripts/run-tests.sh
[warning] 18-18: YELLOW appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (18)
test/e2e/testdata/aws_secret_store_namespace.yaml (1)
1-20: LGTM with template substitution assumption.The manifest uses placeholder syntax consistently with other test data files. Assuming there's a template substitution mechanism in the test framework, the structure is correct for namespace-scoped AWS SecretStore testing.
Note: This assumes template substitution happens before applying to the cluster. See verification comment on
aws_k8s_secret_multi_key.yamlfor checking the substitution mechanism.test/e2e/testdata/aws_ssm_secret_store.yaml (1)
1-22: LGTM!The ClusterSecretStore manifest correctly configures AWS Parameter Store integration. The hardcoded reference to
aws-credsinkube-systemaligns with the setup script (setup-aws-credentials.sh), providing consistent credential management for E2E tests.test/e2e/scripts/cleanup-credentials.sh (1)
1-54: LGTM!The cleanup script has appropriate error handling with clear status messages. The
cleanup_secretfunction correctly handles both success and missing secret scenarios, making it safe to run repeatedly.test/e2e/scripts/setup-aws-credentials.sh (2)
29-31: LGTM - secure password handling.The script correctly uses
read -spfor the AWS Secret Access Key (suppressing echo), followed by explicit newlines for proper terminal formatting. This prevents credentials from being visible in terminal history or on-screen.
1-69: Interactive design is appropriate for the use case.The script is well-structured for interactive credential setup with good error handling, validation, and user feedback. Note that this script requires interactive input and is not suitable for CI/CD environments without modification (see the CI/CD documentation issue flagged in
README.md).go.mod (1)
282-314: Module dependencies are properly integrated and consistent.Version updates across OpenTelemetry (→1.38.0), Go stdlib extensions (→0.30.0+), gRPC (→1.77.0), and protobuf (→1.36.11) have been successfully resolved.
go mod verifyconfirms the module graph is consistent, andgo.sumcontains all required hash entries with no conflicts detected. Go version 1.24.4 supports these dependency versions. No action required.test/e2e/testdata/aws_k8s_secret_multi_key.yaml (1)
10-12: This is unused test fixture data not integrated into any tests.The file
test/e2e/testdata/aws_k8s_secret_multi_key.yamlexists but is not referenced by any test in the codebase. Bothaws_k8s_secret_multi_key.yamland the relatedaws_external_secret_multi_key.yamlwere added as test fixtures but remain unused. The concerns about placeholder substitution and base64 encoding do not currently apply since the file is not loaded or used by any test.Likely an incorrect or invalid review comment.
test/e2e/testdata/aws_secret_store.yaml (1)
1-22: LGTM!Good improvement to parameterize the region with
${AWS_REGION}instead of hardcoding. This makes the test infrastructure more flexible across different AWS regions.test/e2e/testdata/aws_external_secret_multi_key.yaml (1)
1-29: LGTM!Well-structured manifest for testing multi-key extraction from a single AWS secret. The consistent use of placeholders and labeling pattern aligns with other test manifests.
test/e2e/testdata/aws_external_secret_template.yaml (1)
1-32: LGTM!Good test coverage for template-based secret transformation. The manifest correctly demonstrates generating a config file from extracted secret values.
test/e2e/testdata/aws_external_secret_datafrom.yaml (1)
1-19: LGTM!Clean manifest for testing
dataFromextraction. This correctly exercises the bulk extraction pattern from AWS Secrets Manager.test/e2e/testdata/aws_external_secret_jsonpath.yaml (1)
1-21: LGTM!Good test case for nested JSON property extraction using the dot-notation path syntax.
test/e2e/testdata/aws_external_secret_binary.yaml (1)
1-21: LGTM!Clean manifest for testing binary data extraction from AWS Secrets Manager.
test/e2e/testdata/aws_external_secret_refresh.yaml (1)
1-21: LGTM!The ExternalSecret manifest is well-structured with appropriate placeholders for dynamic substitution. The 30s refresh interval is suitable for testing the secret refresh functionality.
test/e2e/scripts/verify-setup.sh (1)
1-140: LGTM!The verification script is comprehensive and well-structured. It properly checks prerequisites, provides clear feedback with color-coded output, and handles optional credentials appropriately with warnings rather than errors.
test/e2e/e2e_test.go (3)
235-378: LGTM - Basic Operations tests.Well-structured tests covering namespace-scoped SecretStore and binary data handling. Good use of defer for cleanup and Eventually for async assertions.
380-652: LGTM - Advanced Features tests.Comprehensive test coverage for secret refresh, template transformations, dataFrom, and JSON path extraction. The 2-minute timeout for refresh verification (line 454) appropriately accounts for the 30s refresh interval plus buffer.
654-716: LGTM - AWS Parameter Store tests.Good integration test for SSM Parameter Store functionality with proper setup and cleanup.
test/e2e/scripts/README.md
Outdated
| ```yaml | ||
| # Example GitHub Actions | ||
| - name: Setup AWS Credentials | ||
| run: | | ||
| echo "$AWS_ACCESS_KEY_ID" | ./test/e2e/scripts/setup-aws-credentials.sh | ||
| env: | ||
| AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }} | ||
| AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }} | ||
|
|
||
| - name: Run E2E Tests | ||
| run: ./test/e2e/scripts/run-tests.sh aws -v | ||
| ``` |
There was a problem hiding this comment.
Fix the CI/CD integration example.
The GitHub Actions example won't work because setup-aws-credentials.sh uses interactive prompts (read -p), but the example pipes credentials via echo. The script needs modification to support non-interactive CI/CD environments.
🔎 Recommended approach
Either:
- Modify the setup script to accept environment variables directly, or
- Update the documentation to show direct
kubectl create secretcommands for CI/CD:
# Example GitHub Actions (corrected)
- name: Setup AWS Credentials
run: |
kubectl create secret generic aws-creds \
--from-literal=aws_access_key_id="${{ secrets.AWS_ACCESS_KEY_ID }}" \
--from-literal=aws_secret_access_key="${{ secrets.AWS_SECRET_ACCESS_KEY }}" \
-n kube-system
env:
AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }}
AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
- name: Run E2E Tests
run: ./test/e2e/scripts/run-tests.sh aws -v
env:
E2E_AWS_REGION: us-east-1🤖 Prompt for AI Agents
In test/e2e/scripts/README.md around lines 219-230, the GitHub Actions example
fails because setup-aws-credentials.sh uses interactive read prompts while the
example pipes values; update the setup script to support non-interactive CI by
reading AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment variables (use
them if set, otherwise fall back to interactive prompt), and update the README
example to either (a) call the script with those env vars exported in the
workflow or (b) replace the step with a kubectl create secret ... example that
injects the GitHub secrets directly into the cluster for CI; ensure the README
shows the corresponding env: entries for the Run E2E Tests step when necessary.
test/e2e/scripts/run-tests.sh
Outdated
| if [ "$VERBOSE" = "-v" ] || [ "$VERBOSE" = "--verbose" ]; then | ||
| make test-e2e E2E_GINKGO_LABEL_FILTER="$label" || return 1 | ||
| else | ||
| make test-e2e E2E_GINKGO_LABEL_FILTER="$label" 2>&1 | grep -E "Ran|PASS|FAIL|SUCCESS|•" || return 1 | ||
| fi |
There was a problem hiding this comment.
Potential false failure in non-verbose mode due to grep exit code.
When $VERBOSE is not set, the script pipes make test-e2e output through grep. If grep finds no matching lines, it exits with code 1, causing return 1 even if the tests passed. This could mask successful test runs or incorrectly report failures.
🔎 Proposed fix
if [ "$VERBOSE" = "-v" ] || [ "$VERBOSE" = "--verbose" ]; then
make test-e2e E2E_GINKGO_LABEL_FILTER="$label" || return 1
else
- make test-e2e E2E_GINKGO_LABEL_FILTER="$label" 2>&1 | grep -E "Ran|PASS|FAIL|SUCCESS|•" || return 1
+ make test-e2e E2E_GINKGO_LABEL_FILTER="$label" 2>&1 | { grep -E "Ran|PASS|FAIL|SUCCESS|•" || true; }
+ return ${PIPESTATUS[0]}
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ "$VERBOSE" = "-v" ] || [ "$VERBOSE" = "--verbose" ]; then | |
| make test-e2e E2E_GINKGO_LABEL_FILTER="$label" || return 1 | |
| else | |
| make test-e2e E2E_GINKGO_LABEL_FILTER="$label" 2>&1 | grep -E "Ran|PASS|FAIL|SUCCESS|•" || return 1 | |
| fi | |
| if [ "$VERBOSE" = "-v" ] || [ "$VERBOSE" = "--verbose" ]; then | |
| make test-e2e E2E_GINKGO_LABEL_FILTER="$label" || return 1 | |
| else | |
| make test-e2e E2E_GINKGO_LABEL_FILTER="$label" 2>&1 | { grep -E "Ran|PASS|FAIL|SUCCESS|•" || true; } | |
| return ${PIPESTATUS[0]} | |
| fi |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/e2e/README.md (1)
9-27: Documentation still has inconsistencies with actual implementation.The test counts and file references remain inconsistent:
- Line 9: "13 test cases"
- Line 19: references
e2e_aws_test.go(file doesn't exist - tests are ine2e_test.go)- Line 107: "11 new AWS tests (plus 1 existing = 12 total)"
- PR description: "8 tests"
Please reconcile the documentation to match the actual implementation.
test/e2e/scripts/README.md (1)
219-230: CI/CD integration example still has the same issue.The GitHub Actions example pipes credentials via
echobutsetup-aws-credentials.shlikely uses interactive prompts (read -p), which won't work in CI. The documentation should show directkubectl create secretcommands for CI environments.
🧹 Nitpick comments (4)
test/e2e/testdata/aws_external_secret_multi_key.yaml (1)
1-29: LGTM! Consider adding documentation for test clarity.The ExternalSecret manifest is correctly structured and follows standard ESO conventions. The use of template variables and property-based extraction from a JSON secret is appropriate for multi-key testing.
Optional: Add comment header for maintainability
Consider adding a brief header comment to document the test scenario and expected secret structure:
+# Test Case: Multi-key extraction from a single AWS Secrets Manager JSON secret +# Expects AWS_SECRET_KEY_NAME to reference a JSON secret with structure: +# { "key1": "value1", "key2": "value2", "key3": "value3" } + apiVersion: external-secrets.io/v1 kind: ExternalSecretThis helps future maintainers understand the expected AWS secret format.
test/utils/conditions.go (1)
149-203: Consider extracting AWS session creation to reduce duplication.The session creation logic is repeated in
CreateAWSSecret,UpdateAWSSecret, andDeleteAWSSecret. Extracting this to a helper would improve maintainability.🔎 Proposed refactor
+func createAWSSession(ctx context.Context, k8sClient *kubernetes.Clientset, region string) (*session.Session, error) { + id, key, err := fetchAWSCreds(ctx, k8sClient) + if err != nil { + return nil, err + } + + sess, err := session.NewSession(&aws.Config{ + Credentials: awscred.NewCredentials(&awscred.StaticProvider{Value: awscred.Value{ + AccessKeyID: id, + SecretAccessKey: key, + }}), + Region: aws.String(region), + }) + if err != nil { + return nil, fmt.Errorf("failed to create AWS session: %w", err) + } + return sess, nil +} + func CreateAWSSecret(ctx context.Context, k8sClient *kubernetes.Clientset, secretName, secretValue, region string) error { - id, key, err := fetchAWSCreds(ctx, k8sClient) - if err != nil { - return err - } - - sess, err := session.NewSession(&aws.Config{ - Credentials: awscred.NewCredentials(&awscred.StaticProvider{Value: awscred.Value{ - AccessKeyID: id, - SecretAccessKey: key, - }}), - Region: aws.String(region), - }) + sess, err := createAWSSession(ctx, k8sClient, region) if err != nil { - return fmt.Errorf("failed to create AWS session: %w", err) + return err } // ... rest of function }test/e2e/testdata/aws_invalid_secret_store.yaml (1)
13-13: Region is hardcoded while other manifests use${AWS_REGION}.For consistency with other AWS test manifests (
aws_secret_store.yaml,aws_ssm_secret_store.yaml), consider parameterizing the region here as well.🔎 Proposed fix
- region: ap-south-1 + region: ${AWS_REGION}test/e2e/scripts/run-tests.sh (1)
16-19: Remove unused YELLOW variable.The
YELLOWvariable is defined but never used in the script. Consider removing it to keep the code clean.🔎 Proposed fix
# Colors GREEN='\033[0;32m' -YELLOW='\033[1;33m' NC='\033[0m'As per static analysis hints, this variable appears unused.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (39)
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/go.opentelemetry.io/otel/SECURITY-INSIGHTS.ymlis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/internal/x/README.mdis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/sdk/trace/internal/x/x.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.30.0/MIGRATION.mdis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.30.0/README.mdis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.30.0/attribute_group.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.30.0/doc.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.30.0/exception.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.30.0/metric.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.30.0/schema.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.34.0/error_type.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/MIGRATION.mdis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/README.mdis excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/attribute_group.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/doc.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/error_type.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/exception.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/otelconv/metric.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.37.0/schema.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/hex.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/telemetry/codereview.cfgis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/internal/ctrlflowinternal/ctrlflowinternal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/plusbuild.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/analysis/passes/modernize/stringscut.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/analysis/analyzerutil/doc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/analysis/analyzerutil/extractdoc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/analysis/analyzerutil/readfile.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/analysis/analyzerutil/version.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/analysis/typeindex/typeindex.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/cfginternal/cfginternal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/packagepath/packagepath.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/ssainternal/ssainternal.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/encoding/internal/internal.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/stats/stats.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (20)
test/e2e/README.mdtest/e2e/e2e_test.gotest/e2e/scripts/README.mdtest/e2e/scripts/cleanup-credentials.shtest/e2e/scripts/run-tests.shtest/e2e/scripts/setup-aws-credentials.shtest/e2e/scripts/verify-setup.shtest/e2e/testdata/aws_external_secret_binary.yamltest/e2e/testdata/aws_external_secret_datafrom.yamltest/e2e/testdata/aws_external_secret_jsonpath.yamltest/e2e/testdata/aws_external_secret_multi_key.yamltest/e2e/testdata/aws_external_secret_refresh.yamltest/e2e/testdata/aws_external_secret_template.yamltest/e2e/testdata/aws_invalid_secret_store.yamltest/e2e/testdata/aws_k8s_secret_multi_key.yamltest/e2e/testdata/aws_secret_store.yamltest/e2e/testdata/aws_secret_store_namespace.yamltest/e2e/testdata/aws_ssm_external_secret.yamltest/e2e/testdata/aws_ssm_secret_store.yamltest/utils/conditions.go
🚧 Files skipped from review as they are similar to previous changes (9)
- test/e2e/testdata/aws_k8s_secret_multi_key.yaml
- test/e2e/scripts/setup-aws-credentials.sh
- test/e2e/testdata/aws_external_secret_datafrom.yaml
- test/e2e/scripts/verify-setup.sh
- test/e2e/testdata/aws_external_secret_refresh.yaml
- test/e2e/testdata/aws_external_secret_template.yaml
- test/e2e/testdata/aws_external_secret_binary.yaml
- test/e2e/testdata/aws_external_secret_jsonpath.yaml
- test/e2e/testdata/aws_ssm_external_secret.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/testdata/aws_ssm_secret_store.yamltest/e2e/scripts/run-tests.shtest/utils/conditions.gotest/e2e/testdata/aws_secret_store.yamltest/e2e/scripts/cleanup-credentials.shtest/e2e/testdata/aws_external_secret_multi_key.yamltest/e2e/e2e_test.gotest/e2e/testdata/aws_invalid_secret_store.yamltest/e2e/testdata/aws_secret_store_namespace.yamltest/e2e/README.mdtest/e2e/scripts/README.md
🧬 Code graph analysis (1)
test/e2e/e2e_test.go (2)
test/utils/conditions.go (6)
ReplacePatternInAsset(249-260)GetRandomString(239-247)DeleteAWSSecret(205-231)CreateAWSSecret(149-175)WaitForESOResourceReady(100-137)UpdateAWSSecret(177-203)api/v1alpha1/groupversion_info.go (1)
Resource(40-42)
🪛 LanguageTool
test/e2e/README.md
[grammar] ~90-~90: Use a hyphen to join words.
Context: ...scoped SecretStore** - Tests non-cluster scoped SecretStore 2. **Multiple data ke...
(QB_NEW_EN_HYPHEN)
test/e2e/scripts/README.md
[style] ~210-~210: ‘without warning’ might be wordy. Consider a shorter alternative.
Context: ...pts will overwrite existing credentials without warning - Use cleanup-credentials.sh to remov...
(EN_WORDINESS_PREMIUM_WITHOUT_WARNING)
🪛 markdownlint-cli2 (0.18.1)
test/e2e/README.md
11-11: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
test/e2e/scripts/README.md
11-11: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
test/e2e/scripts/run-tests.sh
[warning] 18-18: YELLOW appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (8)
test/e2e/testdata/aws_secret_store_namespace.yaml (1)
1-20: LGTM!The namespace-scoped SecretStore manifest is well-structured with appropriate placeholders for templating. The authentication configuration correctly references the secret containing AWS credentials.
test/e2e/testdata/aws_secret_store.yaml (1)
12-12: LGTM!Good change to parameterize the region using
${AWS_REGION}instead of a hardcoded value. This aligns with the other test manifests and enables flexible region configuration.test/e2e/testdata/aws_ssm_secret_store.yaml (1)
1-22: LGTM!The ClusterSecretStore manifest for AWS Parameter Store is well-structured and follows the same parameterization pattern as other AWS manifests.
test/e2e/scripts/cleanup-credentials.sh (1)
1-54: LGTM!The cleanup script is well-structured with appropriate error handling for missing kubectl and graceful handling of non-existent secrets. The provider-based case statement cleanly supports selective or full cleanup.
test/e2e/e2e_test.go (4)
25-25: LGTM!The
encoding/jsonimport is correctly added to support JSON marshaling in the nested JSON test case (line 594).
175-178: LGTM!Excellent improvement to dynamically inject the AWS region into the ClusterSecretStore asset. This makes the tests more maintainable and region-aware.
235-378: LGTM!The Basic Operations test context provides good coverage for:
- Namespace-scoped SecretStore functionality
- Binary data handling with base64 encoding
Both tests follow best practices with proper cleanup (defer), async verification (Eventually), and reasonable timeouts.
383-651: LGTM on test structure!The Advanced Features test context provides excellent coverage for:
- Secret refresh/rotation with configurable intervals
- Template transformations for config generation
- DataFrom for bulk secret import
- JSONPath extraction for nested JSON values
All four tests follow best practices with proper cleanup, async verification, and appropriate timeouts. The only concern is the region inconsistency flagged separately.
test/e2e/e2e_test.go
Outdated
| }) | ||
|
|
||
| Context("Advanced Features", Label("Platform:AWS"), func() { | ||
| const awsRegion = "ap-south-1" |
There was a problem hiding this comment.
Region inconsistency across test contexts.
The Advanced Features context uses "ap-south-1" (line 381), while the Basic Operations context uses "us-east-2" (line 236) and the original AWS Secret Manager context uses "us-east-2" (line 143). This inconsistency could cause test failures if AWS credentials don't have access to all regions or if there are region-specific quotas/limits.
Recommend using a consistent region across all AWS test contexts.
🔎 Proposed fix
Context("Advanced Features", Label("Platform:AWS"), func() {
- const awsRegion = "ap-south-1"
+ const awsRegion = "us-east-2"📝 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.
| const awsRegion = "ap-south-1" | |
| const awsRegion = "us-east-2" |
🤖 Prompt for AI Agents
In test/e2e/e2e_test.go around line 381, the Advanced Features context sets
awsRegion to "ap-south-1" which is inconsistent with other contexts using
"us-east-2"; change this to the same region used elsewhere (e.g., "us-east-2")
and refactor by introducing a single shared awsRegion constant at the top of the
test file (or a test helper) and replace all hard-coded region strings to
reference that constant so all AWS test contexts use the same region.
test/e2e/README.md
Outdated
| ## CI/CD Integration | ||
|
|
||
| Example GitHub Actions workflow: | ||
|
|
||
| ```yaml | ||
| name: E2E Tests | ||
|
|
||
| on: [push, pull_request] | ||
|
|
||
| jobs: | ||
| e2e-aws: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: '1.23' | ||
|
|
||
| - name: Create kind cluster | ||
| run: | | ||
| kind create cluster | ||
| kubectl cluster-info | ||
|
|
||
| - name: Install External Secrets Operator | ||
| run: | | ||
| # Install operator | ||
|
|
||
| - name: Create AWS credentials | ||
| run: | | ||
| kubectl create secret generic aws-creds \ | ||
| --from-literal=aws_access_key_id=${{ secrets.AWS_ACCESS_KEY_ID }} \ | ||
| --from-literal=aws_secret_access_key=${{ secrets.AWS_SECRET_ACCESS_KEY }} \ | ||
| -n kube-system | ||
|
|
||
| - name: Run AWS E2E tests | ||
| run: make test-e2e E2E_GINKGO_LABEL_FILTER="Platform:AWS" |
There was a problem hiding this comment.
nit: Do we need CI/CD Integration section?
test/e2e/e2e_test.go
Outdated
| awsSecretValuePattern = "${SECRET_VALUE}" | ||
| awsClusterSecretStoreNamePattern = "${CLUSTERSECRETSTORE_NAME}" | ||
| awsSecretRegionName = "ap-south-1" | ||
| awsSecretRegionName = "us-east-2" |
There was a problem hiding this comment.
Can we use ap-south-1 or any other Asian region is also fine. It will be less loaded and low latency I feel.
test/e2e/e2e_test.go
Outdated
| }) | ||
|
|
||
| Context("Basic Operations", Label("Platform:AWS"), func() { | ||
| const awsRegion = "us-east-2" |
There was a problem hiding this comment.
Can we use the global const awsSecretRegionName unless explicitly required to use a different one.
test/e2e/scripts/README.md
Outdated
| - Never commit credentials to version control | ||
| - Consider using temporary/test-only credentials with minimal permissions | ||
|
|
||
| ## CI/CD Integration |
There was a problem hiding this comment.
Same here too, world we need this section?
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@test/e2e/e2e_test.go`:
- Around line 590-591: Change the ignored json.Marshal error to be handled: call
json.Marshal(nestedSecret) into (secretBytes, err), check err and fail the test
on error (e.g., t.Fatalf or require.NoError) before converting secretBytes to
string for secretValue; update the code around nestedSecret/secretValue to use
the new err variable so marshaling failures are surfaced.
In `@test/e2e/README.md`:
- Around line 402-406: The README's "AWS Region Issues" section currently states
the default region is "us-east-2" but the test scripts and test code use
"ap-south-1"; update the documentation so both places match by setting the
documented default region to "ap-south-1" (or change the test scripts to use
"us-east-2" if you prefer the other default). Locate the "AWS Region Issues"
heading and the default region string in test/e2e/README.md and make the same
change in test/e2e/scripts/README.md (and any other README strings mentioning
the default region) so all references to the default AWS region are consistent
with the value used by the test code.
In `@test/e2e/scripts/run-tests.sh`:
- Around line 16-19: Remove the unused YELLOW variable definition from the
script: delete the line defining YELLOW='\033[1;33m' (the YELLOW symbol), and
verify there are no remaining references to YELLOW elsewhere in the file (if any
exist, replace them with an appropriate existing color like GREEN or NC); keep
the other color variables (GREEN, NC) intact.
♻️ Duplicate comments (1)
test/e2e/scripts/run-tests.sh (1)
35-39: Potential false failure when grep finds no matches.When not in verbose mode, if
grepfinds no matching lines, it exits with code 1, causing the function to return 1 even if tests passed.🔧 Proposed fix
if [ "$VERBOSE" = "-v" ] || [ "$VERBOSE" = "--verbose" ]; then make test-e2e E2E_GINKGO_LABEL_FILTER="$label" || return 1 else - make test-e2e E2E_GINKGO_LABEL_FILTER="$label" 2>&1 | grep -E "Ran|PASS|FAIL|SUCCESS|•" || return 1 + make test-e2e E2E_GINKGO_LABEL_FILTER="$label" 2>&1 | { grep -E "Ran|PASS|FAIL|SUCCESS|•" || true; } + return ${PIPESTATUS[0]} fi
🧹 Nitpick comments (1)
test/utils/conditions.go (1)
149-231: Consider extracting shared AWS session setup.The session creation logic (fetch credentials → create session → create service client) is duplicated across
CreateAWSSecret,UpdateAWSSecret, andDeleteAWSSecret. A helper function could reduce duplication.This is optional since it's test utility code and the current approach is clear and maintainable.
♻️ Optional refactor
func getSecretsManagerClient(ctx context.Context, k8sClient *kubernetes.Clientset, region string) (*secretsmanager.SecretsManager, error) { id, key, err := fetchAWSCreds(ctx, k8sClient) if err != nil { return nil, err } sess, err := session.NewSession(&aws.Config{ Credentials: awscred.NewCredentials(&awscred.StaticProvider{Value: awscred.Value{ AccessKeyID: id, SecretAccessKey: key, }}), Region: aws.String(region), }) if err != nil { return nil, fmt.Errorf("failed to create AWS session: %w", err) } return secretsmanager.New(sess), nil }
| secretBytes, _ := json.Marshal(nestedSecret) | ||
| secretValue := string(secretBytes) |
There was a problem hiding this comment.
Handle json.Marshal error instead of ignoring it.
Ignoring the error from json.Marshal could mask issues. While marshaling this simple map is unlikely to fail, explicitly handling the error makes the test more robust.
🔧 Proposed fix
- secretBytes, _ := json.Marshal(nestedSecret)
- secretValue := string(secretBytes)
+ secretBytes, err := json.Marshal(nestedSecret)
+ Expect(err).NotTo(HaveOccurred(), "failed to marshal nested secret")
+ secretValue := string(secretBytes)📝 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.
| secretBytes, _ := json.Marshal(nestedSecret) | |
| secretValue := string(secretBytes) | |
| secretBytes, err := json.Marshal(nestedSecret) | |
| Expect(err).NotTo(HaveOccurred(), "failed to marshal nested secret") | |
| secretValue := string(secretBytes) |
🤖 Prompt for AI Agents
In `@test/e2e/e2e_test.go` around lines 590 - 591, Change the ignored json.Marshal
error to be handled: call json.Marshal(nestedSecret) into (secretBytes, err),
check err and fail the test on error (e.g., t.Fatalf or require.NoError) before
converting secretBytes to string for secretValue; update the code around
nestedSecret/secretValue to use the new err variable so marshaling failures are
surfaced.
test/e2e/scripts/run-tests.sh
Outdated
| # Colors | ||
| GREEN='\033[0;32m' | ||
| YELLOW='\033[1;33m' | ||
| NC='\033[0m' |
There was a problem hiding this comment.
Remove unused YELLOW variable.
The YELLOW color variable is defined but never used in the script. Based on static analysis hint.
🔧 Proposed fix
# Colors
GREEN='\033[0;32m'
-YELLOW='\033[1;33m'
NC='\033[0m'📝 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.
| # Colors | |
| GREEN='\033[0;32m' | |
| YELLOW='\033[1;33m' | |
| NC='\033[0m' | |
| # Colors | |
| GREEN='\033[0;32m' | |
| NC='\033[0m' |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 18-18: YELLOW appears unused. Verify use (or export if used externally).
(SC2034)
🤖 Prompt for AI Agents
In `@test/e2e/scripts/run-tests.sh` around lines 16 - 19, Remove the unused YELLOW
variable definition from the script: delete the line defining
YELLOW='\033[1;33m' (the YELLOW symbol), and verify there are no remaining
references to YELLOW elsewhere in the file (if any exist, replace them with an
appropriate existing color like GREEN or NC); keep the other color variables
(GREEN, NC) intact.
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@test/e2e/README.md`:
- Around line 447-449: Update the example to use the correct AWS region to match
the test implementation: change the hardcoded region string "us-east-2" to
"ap-south-1" in both the utils.DeleteAWSSecret and utils.CreateAWSSecret calls
(the calls with identifiers secretName, utils.DeleteAWSSecret and
utils.CreateAWSSecret) so the example and test use the same region.
In `@test/e2e/scripts/README.md`:
- Line 172: Replace the bare URL in the README line "Install kubectl:
https://kubernetes.io/docs/tasks/tools/" with a proper markdown link or wrap it
in angle brackets to satisfy MD034; for example, change it to "Install kubectl:
<https://kubernetes.io/docs/tasks/tools/>" or "Install kubectl: [Kubernetes
install kubectl](https://kubernetes.io/docs/tasks/tools/)". Ensure the edited
line is the one containing the text "Install kubectl:
https://kubernetes.io/docs/tasks/tools/" so markdownlint MD034 is resolved.
In `@test/e2e/scripts/verify-setup.sh`:
- Around line 61-68: The pod count logic in the verify-setup.sh snippet
(variable OPERATOR_PODS) currently counts all pods regardless of status; change
the kubectl pipeline to count only pods in the "Running" state (e.g., filter the
kubectl get pods output for "Running" before wc -l) and ensure the subsequent
detailed listing (the kubectl get pods ... | grep -E "Running|NAME") matches
that same filter so the echoed "Running" message and count reflect only Running
pods; update references to OPERATOR_PODS and the two kubectl get pods commands
accordingly.
♻️ Duplicate comments (4)
test/e2e/e2e_test.go (1)
590-591: Handlejson.Marshalerror instead of ignoring it.The error from
json.Marshalis being discarded. While unlikely to fail for this simple map, explicitly handling the error makes the test more robust and follows best practices.🔧 Proposed fix
- secretBytes, _ := json.Marshal(nestedSecret) - secretValue := string(secretBytes) + secretBytes, err := json.Marshal(nestedSecret) + Expect(err).NotTo(HaveOccurred(), "failed to marshal nested secret") + secretValue := string(secretBytes)test/e2e/scripts/run-tests.sh (2)
16-19: Remove unusedYELLOWvariable.The
YELLOWcolor variable is defined but never used in the script. Based on static analysis.🔧 Proposed fix
# Colors GREEN='\033[0;32m' -YELLOW='\033[1;33m' NC='\033[0m'
35-39: Potential false failure due to grep exit code.When
$VERBOSEis not set, the script pipes output throughgrep. Ifgrepfinds no matching lines, it exits with code 1, causingreturn 1even if the tests passed. This could incorrectly report failures.🔧 Proposed fix
if [ "$VERBOSE" = "-v" ] || [ "$VERBOSE" = "--verbose" ]; then make test-e2e E2E_GINKGO_LABEL_FILTER="$label" || return 1 else - make test-e2e E2E_GINKGO_LABEL_FILTER="$label" 2>&1 | grep -E "Ran|PASS|FAIL|SUCCESS|•" || return 1 + local exit_code + set +e + make test-e2e E2E_GINKGO_LABEL_FILTER="$label" 2>&1 | grep -E "Ran|PASS|FAIL|SUCCESS|•" + exit_code=${PIPESTATUS[0]} + set -e + return $exit_code fitest/e2e/README.md (1)
402-406: Inconsistent region documentation.Line 405 states the default region is
us-east-2, but the test code usesap-south-1(as defined byawsSecretRegionNameconstant). Please align the documentation with the actual implementation.📝 Suggested fix
### AWS Region Issues **Error**: Tests fail with "region not found" or similar AWS errors -**Solution**: Verify the AWS region in your SecretStore/ClusterSecretStore configuration matches your AWS setup. Default region is `us-east-2`. +**Solution**: Verify the AWS region in your SecretStore/ClusterSecretStore configuration matches your AWS setup. Default region is `ap-south-1`.
🧹 Nitpick comments (3)
test/e2e/testdata/aws_invalid_secret_store.yaml (1)
11-13: Consider parameterizing the region for consistency.Align this manifest with the rest of the AWS test data by using the region placeholder so tests respect the configured AWS region.
♻️ Suggested change
- region: ap-south-1 + region: ${AWS_REGION}test/utils/conditions.go (1)
149-203: Consider extracting AWS session creation to reduce duplication.
CreateAWSSecret,UpdateAWSSecret, andDeleteAWSSecretall duplicate the same session creation logic (fetch creds, create session). Extracting this to a helper function would improve maintainability.♻️ Suggested refactor
func newAWSSecretsManagerClient(ctx context.Context, k8sClient *kubernetes.Clientset, region string) (*secretsmanager.SecretsManager, error) { id, key, err := fetchAWSCreds(ctx, k8sClient) if err != nil { return nil, err } sess, err := session.NewSession(&aws.Config{ Credentials: awscred.NewCredentials(&awscred.StaticProvider{Value: awscred.Value{ AccessKeyID: id, SecretAccessKey: key, }}), Region: aws.String(region), }) if err != nil { return nil, fmt.Errorf("failed to create AWS session: %w", err) } return secretsmanager.New(sess), nil } func CreateAWSSecret(ctx context.Context, k8sClient *kubernetes.Clientset, secretName, secretValue, region string) error { svc, err := newAWSSecretsManagerClient(ctx, k8sClient, region) if err != nil { return err } _, err = svc.CreateSecret(&secretsmanager.CreateSecretInput{ Name: aws.String(secretName), SecretString: aws.String(secretValue), }) if err != nil { return fmt.Errorf("failed to create AWS secret: %w", err) } return nil }test/e2e/scripts/verify-setup.sh (1)
36-36:--shortflag is deprecated and removed in kubectl v1.30+.This will produce deprecation warnings or fail silently on newer kubectl versions. Consider using the JSON output format instead.
Suggested fix
- kubectl version --short 2>/dev/null | grep "Server Version" || true + kubectl version -o json 2>/dev/null | jq -r '"Server Version: " + .serverVersion.gitVersion' || true
test/e2e/README.md
Outdated
| defer utils.DeleteAWSSecret(ctx, "us-east-2", secretName) | ||
|
|
||
| err := utils.CreateAWSSecret(ctx, "us-east-2", secretName, `{"key":"value"}`) |
There was a problem hiding this comment.
Example uses incorrect region.
The example code uses us-east-2 but should use ap-south-1 to match the actual test implementation.
📝 Suggested fix
// Create AWS secret
secretName := fmt.Sprintf("eso-e2e-%s", utils.GetRandomString(8))
- defer utils.DeleteAWSSecret(ctx, "us-east-2", secretName)
+ defer utils.DeleteAWSSecret(ctx, "ap-south-1", secretName)
- err := utils.CreateAWSSecret(ctx, "us-east-2", secretName, `{"key":"value"}`)
+ err := utils.CreateAWSSecret(ctx, "ap-south-1", secretName, `{"key":"value"}`)📝 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.
| defer utils.DeleteAWSSecret(ctx, "us-east-2", secretName) | |
| err := utils.CreateAWSSecret(ctx, "us-east-2", secretName, `{"key":"value"}`) | |
| // Create AWS secret | |
| secretName := fmt.Sprintf("eso-e2e-%s", utils.GetRandomString(8)) | |
| defer utils.DeleteAWSSecret(ctx, "ap-south-1", secretName) | |
| err := utils.CreateAWSSecret(ctx, "ap-south-1", secretName, `{"key":"value"}`) |
🤖 Prompt for AI Agents
In `@test/e2e/README.md` around lines 447 - 449, Update the example to use the
correct AWS region to match the test implementation: change the hardcoded region
string "us-east-2" to "ap-south-1" in both the utils.DeleteAWSSecret and
utils.CreateAWSSecret calls (the calls with identifiers secretName,
utils.DeleteAWSSecret and utils.CreateAWSSecret) so the example and test use the
same region.
test/e2e/scripts/README.md
Outdated
| ## Troubleshooting | ||
|
|
||
| ### kubectl not found | ||
| Install kubectl: https://kubernetes.io/docs/tasks/tools/ |
There was a problem hiding this comment.
Fix markdownlint MD034 (bare URL).
Wrap the URL in angle brackets or a markdown link to avoid lint failures.
🛠️ Suggested fix
-Install kubectl: https://kubernetes.io/docs/tasks/tools/
+Install kubectl: <https://kubernetes.io/docs/tasks/tools/>📝 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.
| Install kubectl: https://kubernetes.io/docs/tasks/tools/ | |
| Install kubectl: <https://kubernetes.io/docs/tasks/tools/> |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
172-172: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In `@test/e2e/scripts/README.md` at line 172, Replace the bare URL in the README
line "Install kubectl: https://kubernetes.io/docs/tasks/tools/" with a proper
markdown link or wrap it in angle brackets to satisfy MD034; for example, change
it to "Install kubectl: <https://kubernetes.io/docs/tasks/tools/>" or "Install
kubectl: [Kubernetes install kubectl](https://kubernetes.io/docs/tasks/tools/)".
Ensure the edited line is the one containing the text "Install kubectl:
https://kubernetes.io/docs/tasks/tools/" so markdownlint MD034 is resolved.
test/e2e/scripts/verify-setup.sh
Outdated
| OPERATOR_PODS=$(kubectl get pods -n external-secrets-operator 2>/dev/null | grep -v NAME | wc -l | tr -d ' ') | ||
| if [ "$OPERATOR_PODS" -gt 0 ]; then | ||
| echo -e "${GREEN}✓ Running ($OPERATOR_PODS pods)${NC}" | ||
| kubectl get pods -n external-secrets-operator 2>/dev/null | grep -E "Running|NAME" || true | ||
| else | ||
| echo -e "${RED}✗ No pods found${NC}" | ||
| READY=false | ||
| fi |
There was a problem hiding this comment.
Pod count includes non-running pods but reports them as "Running".
The count at line 61 includes all pods regardless of status, but the output claims they are "Running". Pods in CrashLoopBackOff, Pending, or Error states would be counted and reported as running, potentially masking deployment issues.
Suggested fix
-OPERATOR_PODS=$(kubectl get pods -n external-secrets-operator 2>/dev/null | grep -v NAME | wc -l | tr -d ' ')
-if [ "$OPERATOR_PODS" -gt 0 ]; then
- echo -e "${GREEN}✓ Running ($OPERATOR_PODS pods)${NC}"
+OPERATOR_RUNNING=$(kubectl get pods -n external-secrets-operator --field-selector=status.phase=Running 2>/dev/null | grep -v NAME | wc -l | tr -d ' ')
+OPERATOR_TOTAL=$(kubectl get pods -n external-secrets-operator 2>/dev/null | grep -v NAME | wc -l | tr -d ' ')
+if [ "$OPERATOR_RUNNING" -gt 0 ]; then
+ echo -e "${GREEN}✓ Running ($OPERATOR_RUNNING/$OPERATOR_TOTAL pods)${NC}"
kubectl get pods -n external-secrets-operator 2>/dev/null | grep -E "Running|NAME" || true
+elif [ "$OPERATOR_TOTAL" -gt 0 ]; then
+ echo -e "${RED}✗ Pods exist but none running ($OPERATOR_TOTAL pods)${NC}"
+ kubectl get pods -n external-secrets-operator 2>/dev/null || true
+ READY=false
else
echo -e "${RED}✗ No pods found${NC}"
READY=false
fi🤖 Prompt for AI Agents
In `@test/e2e/scripts/verify-setup.sh` around lines 61 - 68, The pod count logic
in the verify-setup.sh snippet (variable OPERATOR_PODS) currently counts all
pods regardless of status; change the kubectl pipeline to count only pods in the
"Running" state (e.g., filter the kubectl get pods output for "Running" before
wc -l) and ensure the subsequent detailed listing (the kubectl get pods ... |
grep -E "Running|NAME") matches that same filter so the echoed "Running" message
and count reflect only Running pods; update references to OPERATOR_PODS and the
two kubectl get pods commands accordingly.
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/lgtm |
|
@emmajiafan: you cannot LGTM your own PR. 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 kubernetes-sigs/prow repository. |
openshift-ci
bot
added
qe-approved
|
@emmajiafan: The label(s) 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 kubernetes-sigs/prow repository. |
|
/label docs-approved |
openshift-ci
bot
added
the
docs-approved
test/e2e/README.md
Outdated
There was a problem hiding this comment.
I'm not against having a README for the e2e tests, but could we avoid committing AI-generated intermediate "reports" or "summary", since they would become outdated frequently and would only increase the maintenance burden. :)
test/e2e/scripts/README.md
Outdated
There was a problem hiding this comment.
Could we remove the scripts folder from this PR, since these AI-generated "script docs" look pretty random and redundant. If we really want to add some handy scripts, we should revise them to not introduce much noise.
remove the ssm related update the region update the vendor rmove the ai generate scripts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/utils/conditions.go`:
- Around line 149-203: Both CreateAWSSecret and UpdateAWSSecret accept a ctx but
call the blocking SDK methods; update each to use the context-aware variants by
replacing svc.CreateSecret(...) with svc.CreateSecretWithContext(ctx, ...) in
CreateAWSSecret and svc.UpdateSecret(...) with svc.UpdateSecretWithContext(ctx,
...) in UpdateAWSSecret so cancellations/timeouts propagate; keep the same
inputs/outputs and error wrapping around the calls.
🧹 Nitpick comments (1)
test/e2e/e2e_test.go (1)
21-28: Allow AWS region to be overridden for test portability.Hard-coding a region can make these tests brittle across environments. Consider an env override with a default.
♻️ Suggested refactor
import ( "context" "embed" "encoding/base64" "encoding/json" + "os" "fmt" "testing" "time" @@ - awsSecretRegionName = "ap-south-1" ) + +var awsSecretRegionName = func() string { + if v := os.Getenv("AWS_REGION"); v != "" { + return v + } + return "ap-south-1" +}()Also applies to: 62-70
| func CreateAWSSecret(ctx context.Context, k8sClient *kubernetes.Clientset, secretName, secretValue, region string) error { | ||
| id, key, err := fetchAWSCreds(ctx, k8sClient) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| sess, err := session.NewSession(&aws.Config{ | ||
| Credentials: awscred.NewCredentials(&awscred.StaticProvider{Value: awscred.Value{ | ||
| AccessKeyID: id, | ||
| SecretAccessKey: key, | ||
| }}), | ||
| Region: aws.String(region), | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create AWS session: %w", err) | ||
| } | ||
|
|
||
| svc := secretsmanager.New(sess) | ||
| _, err = svc.CreateSecret(&secretsmanager.CreateSecretInput{ | ||
| Name: aws.String(secretName), | ||
| SecretString: aws.String(secretValue), | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create AWS secret: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func UpdateAWSSecret(ctx context.Context, k8sClient *kubernetes.Clientset, secretName, secretValue, region string) error { | ||
| id, key, err := fetchAWSCreds(ctx, k8sClient) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| sess, err := session.NewSession(&aws.Config{ | ||
| Credentials: awscred.NewCredentials(&awscred.StaticProvider{Value: awscred.Value{ | ||
| AccessKeyID: id, | ||
| SecretAccessKey: key, | ||
| }}), | ||
| Region: aws.String(region), | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create AWS session: %w", err) | ||
| } | ||
|
|
||
| svc := secretsmanager.New(sess) | ||
| _, err = svc.UpdateSecret(&secretsmanager.UpdateSecretInput{ | ||
| SecretId: aws.String(secretName), | ||
| SecretString: aws.String(secretValue), | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update AWS secret: %w", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -path "*/test/utils/conditions.go" -type fRepository: openshift/external-secrets-operator
Length of output: 105
🏁 Script executed:
# If file exists, check its contents around lines 149-203
if [ -f "test/utils/conditions.go" ]; then
wc -l test/utils/conditions.go
echo "---"
sed -n '140,210p' test/utils/conditions.go
fiRepository: openshift/external-secrets-operator
Length of output: 2146
🌐 Web query:
AWS SDK Go secretsmanager CreateSecretWithContext UpdateSecretWithContext methods
💡 Result:
In AWS SDK for Go v1 (github.com/aws/aws-sdk-go/service/secretsmanager), the WithContext variants are the same API calls as CreateSecret / UpdateSecret, but let you pass a non-nil context for cancellation/timeouts plus optional request options:
func (c *SecretsManager) CreateSecretWithContext(ctx aws.Context, input *CreateSecretInput, opts ...request.Option) (*CreateSecretOutput, error)(docs.aws.amazon.com)func (c *SecretsManager) UpdateSecretWithContext(ctx aws.Context, input *UpdateSecretInput, opts ...request.Option) (*UpdateSecretOutput, error)(docs.aws.amazon.com)
Example (v1):
sess := session.Must(session.NewSession())
sm := secretsmanager.New(sess)
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
_, err := sm.CreateSecretWithContext(ctx, &secretsmanager.CreateSecretInput{
Name: aws.String("my-secret"),
SecretString: aws.String(`{"user":"u","pass":"p"}`),
})
_, err = sm.UpdateSecretWithContext(ctx, &secretsmanager.UpdateSecretInput{
SecretId: aws.String("my-secret"),
SecretString: aws.String(`{"user":"u","pass":"p2"}`),
})In AWS SDK for Go v2 (github.com/aws/aws-sdk-go-v2/service/secretsmanager), the context is already part of the standard method signature (no WithContext suffix):
func (c *Client) CreateSecret(ctx context.Context, params *CreateSecretInput, optFns ...func(*Options)) (*CreateSecretOutput, error)(pkg.go.dev)func (c *Client) UpdateSecret(ctx context.Context, params *UpdateSecretInput, optFns ...func(*Options)) (*UpdateSecretOutput, error)(pkg.go.dev)
Citations:
- 1: https://docs.aws.amazon.com/sdk-for-go/api/service/secretsmanager/?utm_source=openai
- 2: https://docs.aws.amazon.com/sdk-for-go/api/service/secretsmanager/?utm_source=openai
- 3: https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/service/secretsmanager?utm_source=openai
- 4: https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/service/secretsmanager?utm_source=openai
Use context-aware AWS SDK calls to avoid hanging tests.
ctx is accepted by both CreateAWSSecret and UpdateAWSSecret but ignored in the AWS SDK calls. This prevents test timeouts and cancellations from propagating, risking hung tests on network stalls. Use the WithContext variants to respect the provided context.
🛠️ Suggested changes
- _, err = svc.CreateSecret(&secretsmanager.CreateSecretInput{
+ _, err = svc.CreateSecretWithContext(ctx, &secretsmanager.CreateSecretInput{
Name: aws.String(secretName),
SecretString: aws.String(secretValue),
})- _, err = svc.UpdateSecret(&secretsmanager.UpdateSecretInput{
+ _, err = svc.UpdateSecretWithContext(ctx, &secretsmanager.UpdateSecretInput{
SecretId: aws.String(secretName),
SecretString: aws.String(secretValue),
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func CreateAWSSecret(ctx context.Context, k8sClient *kubernetes.Clientset, secretName, secretValue, region string) error { | |
| id, key, err := fetchAWSCreds(ctx, k8sClient) | |
| if err != nil { | |
| return err | |
| } | |
| sess, err := session.NewSession(&aws.Config{ | |
| Credentials: awscred.NewCredentials(&awscred.StaticProvider{Value: awscred.Value{ | |
| AccessKeyID: id, | |
| SecretAccessKey: key, | |
| }}), | |
| Region: aws.String(region), | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("failed to create AWS session: %w", err) | |
| } | |
| svc := secretsmanager.New(sess) | |
| _, err = svc.CreateSecret(&secretsmanager.CreateSecretInput{ | |
| Name: aws.String(secretName), | |
| SecretString: aws.String(secretValue), | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("failed to create AWS secret: %w", err) | |
| } | |
| return nil | |
| } | |
| func UpdateAWSSecret(ctx context.Context, k8sClient *kubernetes.Clientset, secretName, secretValue, region string) error { | |
| id, key, err := fetchAWSCreds(ctx, k8sClient) | |
| if err != nil { | |
| return err | |
| } | |
| sess, err := session.NewSession(&aws.Config{ | |
| Credentials: awscred.NewCredentials(&awscred.StaticProvider{Value: awscred.Value{ | |
| AccessKeyID: id, | |
| SecretAccessKey: key, | |
| }}), | |
| Region: aws.String(region), | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("failed to create AWS session: %w", err) | |
| } | |
| svc := secretsmanager.New(sess) | |
| _, err = svc.UpdateSecret(&secretsmanager.UpdateSecretInput{ | |
| SecretId: aws.String(secretName), | |
| SecretString: aws.String(secretValue), | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("failed to update AWS secret: %w", err) | |
| } | |
| return nil | |
| } | |
| func CreateAWSSecret(ctx context.Context, k8sClient *kubernetes.Clientset, secretName, secretValue, region string) error { | |
| id, key, err := fetchAWSCreds(ctx, k8sClient) | |
| if err != nil { | |
| return err | |
| } | |
| sess, err := session.NewSession(&aws.Config{ | |
| Credentials: awscred.NewCredentials(&awscred.StaticProvider{Value: awscred.Value{ | |
| AccessKeyID: id, | |
| SecretAccessKey: key, | |
| }}), | |
| Region: aws.String(region), | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("failed to create AWS session: %w", err) | |
| } | |
| svc := secretsmanager.New(sess) | |
| _, err = svc.CreateSecretWithContext(ctx, &secretsmanager.CreateSecretInput{ | |
| Name: aws.String(secretName), | |
| SecretString: aws.String(secretValue), | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("failed to create AWS secret: %w", err) | |
| } | |
| return nil | |
| } | |
| func UpdateAWSSecret(ctx context.Context, k8sClient *kubernetes.Clientset, secretName, secretValue, region string) error { | |
| id, key, err := fetchAWSCreds(ctx, k8sClient) | |
| if err != nil { | |
| return err | |
| } | |
| sess, err := session.NewSession(&aws.Config{ | |
| Credentials: awscred.NewCredentials(&awscred.StaticProvider{Value: awscred.Value{ | |
| AccessKeyID: id, | |
| SecretAccessKey: key, | |
| }}), | |
| Region: aws.String(region), | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("failed to create AWS session: %w", err) | |
| } | |
| svc := secretsmanager.New(sess) | |
| _, err = svc.UpdateSecretWithContext(ctx, &secretsmanager.UpdateSecretInput{ | |
| SecretId: aws.String(secretName), | |
| SecretString: aws.String(secretValue), | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("failed to update AWS secret: %w", err) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In `@test/utils/conditions.go` around lines 149 - 203, Both CreateAWSSecret and
UpdateAWSSecret accept a ctx but call the blocking SDK methods; update each to
use the context-aware variants by replacing svc.CreateSecret(...) with
svc.CreateSecretWithContext(ctx, ...) in CreateAWSSecret and
svc.UpdateSecret(...) with svc.UpdateSecretWithContext(ctx, ...) in
UpdateAWSSecret so cancellations/timeouts propagate; keep the same
inputs/outputs and error wrapping around the calls.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@emmajiafan: all tests passed! 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. |
emmajiafan
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@emmajiafan: This pull request explicitly references no jira issue. 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. |
bharath-b-rh
left a comment
bharath-b-rh
left a comment
There was a problem hiding this comment.
Few nits, otherwise LGTM.
@mytreya-rh had brought this up in another PR. We are adding a CR manifest for every scenario, which adds to maintenance burden, and instead we should make use of go API definitions for the same, and derive the required manifests.
| } | ||
|
|
||
| svc := secretsmanager.New(sess) | ||
| _, err = svc.CreateSecret(&secretsmanager.CreateSecretInput{ |
There was a problem hiding this comment.
| _, err = svc.CreateSecret(&secretsmanager.CreateSecretInput{ | |
| _, err = svc.CreateSecretWithContext(ctx, &secretsmanager.CreateSecretInput{ |
| } | ||
|
|
||
| svc := secretsmanager.New(sess) | ||
| _, err = svc.UpdateSecret(&secretsmanager.UpdateSecretInput{ |
There was a problem hiding this comment.
| _, err = svc.UpdateSecret(&secretsmanager.UpdateSecretInput{ | |
| _, err = svc.UpdateSecretWithContext(ctx, &secretsmanager.UpdateSecretInput{ |
4 participants
Test Coverage Summary