SREP-3016: Add functional e2e tests for aws-vpce-operator#375
Open
rpodishe wants to merge 2 commits intoopenshift:mainfrom
Open
SREP-3016: Add functional e2e tests for aws-vpce-operator#375rpodishe wants to merge 2 commits intoopenshift:mainfrom
rpodishe wants to merge 2 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rpodishe: This pull request references SREP-3016 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rpodishe 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 |
WalkthroughAdds comprehensive e2e testing: new Ginkgo/Gomega test suite and helpers, operator subprocess lifecycle for tests, AWS/Kubernetes validation utilities, a single .gitignore entry, and minor CI/build metadata/base-image updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test Suite
participant K8s as Kubernetes API
participant Op as Operator (subprocess)
participant AWS as AWS API
Note over Test,K8s: Setup: register schemes, apply CRDs
Test->>Op: start (if not healthy)
Op->>Test: /healthz OK
Test->>K8s: create `VpcEndpoint` CR
K8s->>Op: watch CR -> reconcile
Op->>AWS: create VPC Endpoint & Security Group
AWS-->>Op: return VPCE ID, SG ID
Op->>K8s: update CR status (IDs, available)
Test->>AWS: verify VPCE & SG exist
Test->>K8s: delete `VpcEndpoint` CR
K8s->>Op: reconcile deletion
Op->>AWS: delete VPCE & SG
AWS-->>Op: confirm deletion
Op->>K8s: remove finalizer -> CR removed
Test->>AWS: assert resources deleted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@rpodishe: This pull request references SREP-3016 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #375 +/- ##
=======================================
Coverage 40.69% 40.69%
=======================================
Files 32 32
Lines 2106 2106
=======================================
Hits 857 857
Misses 1145 1145
Partials 104 104 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/aws_integration_test.go`:
- Around line 281-293: The test's drift check can pass while the original
endpoint is still deleting; update the Eventually block that calls
helper.awsClient.DescribeSingleVPCEndpointById and inspects
obj.Status.VPCEndpointId so it verifies the endpoint is fully usable: require
that the returned VpcEndpointId is different from the previously deleted ID (or
store the deleted ID and compare) and assert the endpoint State equals
"available" (not just != "deleted"); adjust the expectations around
obj.Status.VPCEndpointId and resp.VpcEndpoints[0].State in the Eventually
closure (and keep using vpceReadyTimeout/pollingInterval) so the test waits
until AWS reports the new endpoint as available before succeeding.
In `@test/e2e/aws_vpce_operator_tests.go`:
- Around line 104-117: The AWS profile resolution code using
awsconfig.LoadSharedConfigProfile must also export temporary session tokens:
when you append resolved credentials to operatorCmd.Env in the block that checks
cfg.Credentials.AccessKeyID and cfg.Credentials.SecretAccessKey, also check
cfg.Credentials.SessionToken and append
"AWS_SESSION_TOKEN="+cfg.Credentials.SessionToken if non-empty; likewise update
the getOverrideCredentials() function to return/include the SessionToken it
reads from the profile (and ensure callers propagate it into any env
construction), so temporary STS creds are complete for subprocesses and tests.
In `@test/e2e/helpers_test.go`:
- Around line 155-166: The Eventually body treats SDK errors as success because
it ignores non-nil err; update the polling closure (the Eventually that calls
DescribeSingleVPCEndpointById) to explicitly fail on any error from
DescribeSingleVPCEndpointById by asserting the error (e.g., using Gomega on err
via g.Expect(err).NotTo(HaveOccurred()) or g.Expect(err).To(BeNil())) before
proceeding to check resp, keep the existing nil resp handling and the
resp.VpcEndpoints[0].State == "deleted" assertion, and apply the same
explicit-error assertion to the similar block referencing
DescribeSingleVPCEndpointById at lines 179-188.
- Around line 219-223: When deletion times out in the isVpceDeletedWithin check,
after calling stripFinalizers(ctx, c, name, namespace) you must wait again for
the CR to actually disappear instead of returning immediately; re-run
isVpceDeletedWithin(ctx, c, name, namespace, vpceDeletedTimeout) (or a shorter
retry loop) and only proceed once it returns true (or explicitly fail/log if it
still times out), preserving the existing GinkgoLogr.Info call so the strip
action is logged; update the logic around isVpceDeletedWithin, stripFinalizers,
name, namespace, vpceDeletedTimeout, ctx and c accordingly.
- Around line 192-201: createTestNamespace currently fails on AlreadyExists;
make namespace creation idempotent by calling c.Create(ctx, ns) and checking the
returned error: if err == nil proceed to DeferCleanup to delete the namespace,
if apierrors.IsAlreadyExists(err) simply return (do not call Expect to fail).
Use k8s.io/apimachinery/pkg/api/errors.IsAlreadyExists to detect the condition
and keep DeferCleanup tied only to the successful create path so we don't
attempt to delete namespaces we didn't create; reference createTestNamespace,
c.Create, apierrors.IsAlreadyExists, and DeferCleanup in the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f43074b-1898-4bdb-a098-3925390a1cd3
📒 Files selected for processing (4)
.gitignoretest/e2e/aws_integration_test.gotest/e2e/aws_vpce_operator_tests.gotest/e2e/helpers_test.go
rpodishe
force-pushed
the
SREP-3016-add-functional-tests
branch
from
682344d to
b31f351
Compare
|
@rpodishe: This pull request references SREP-3016 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
|
@rpodishe: 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/helpers_test.go (1)
66-73: Silent fallback may mask test misconfiguration.When
testKeydoesn't exist in the map, the function silently falls back to"ssmmessages". This could hide typos or missing entries without any indication. Consider logging a warning or returning an error for unknown keys.Optional: Add logging for unknown test keys
func testServiceName(region, testKey string) string { svc, ok := testServiceNames[testKey] if !ok { + GinkgoLogr.Info("unknown test key, using default service", "testKey", testKey, "default", "ssmmessages") svc = "ssmmessages" } return fmt.Sprintf("com.amazonaws.%s.%s", region, svc) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/helpers_test.go` around lines 66 - 73, The function testServiceName silently falls back to "ssmmessages" when testKey is missing from testServiceNames which can hide typos; change testServiceName to return (string, error) instead of only string, check the map and if the key is missing return a descriptive error (e.g., fmt.Errorf("unknown test service key: %s", testKey)), otherwise return the formatted service string and nil error; update all callers of testServiceName to handle the error (fail the test or log as appropriate) so missing keys are surfaced rather than silently ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e/helpers_test.go`:
- Around line 66-73: The function testServiceName silently falls back to
"ssmmessages" when testKey is missing from testServiceNames which can hide
typos; change testServiceName to return (string, error) instead of only string,
check the map and if the key is missing return a descriptive error (e.g.,
fmt.Errorf("unknown test service key: %s", testKey)), otherwise return the
formatted service string and nil error; update all callers of testServiceName to
handle the error (fail the test or log as appropriate) so missing keys are
surfaced rather than silently ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b170878c-fdbb-4113-b1de-3fdc84a32587
⛔ Files ignored due to path filters (6)
boilerplate/_data/last-boilerplate-commitis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/OWNERS_ALIASESis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/README.mdis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/ensure.shis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/olm_pko_migration.pyis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/standard.mkis excluded by!boilerplate/**
📒 Files selected for processing (6)
.tekton/aws-vpce-operator-pko-pull-request.yamlOWNERS_ALIASESbuild/Dockerfilebuild/Dockerfile.olm-registrytest/e2e/aws_integration_test.gotest/e2e/helpers_test.go
✅ Files skipped from review due to trivial changes (3)
- build/Dockerfile
- build/Dockerfile.olm-registry
- OWNERS_ALIASES
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
credential override, spec updates, drift detection, finalizer/tagging validation, and error scenarios
operator behavior against live AWS resources
functions, and unique service name mapping to avoid private-DNS conflicts
and manage the full test lifecycle automatically
Summary by CodeRabbit