ROSAENG-62387 | fix(test): align OCP-45509 proxy assertion and skip on expired AWS creds#3417
ROSAENG-62387 | fix(test): align OCP-45509 proxy assertion and skip on expired AWS creds#3417amandahla wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe coverage script excludes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amandahla The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/test_rosacli_cluster.go`:
- Line 1275: Make teardown safe when BeforeEach skips setup: in
tests/e2e/test_rosacli_cluster.go at lines 1275-1275, 2659-2659, and 3191-3191,
guard clusterHandler.Destroy() so it runs only when cleanup state is
initialized; at line 2386-2386, reset accountRolePrefixToClean before the guard
and perform cleanup only when rosaClient is initialized.
In `@tests/utils/helper/aws_test.go`:
- Around line 3-150: Convert the tests in TestIsAWSCredentialError,
TestAWSCredentialsInvalid, and TestSkipIfAWSCredentialsInvalid from testing.T
subtests and Fatalf checks to the repository’s Ginkgo v2 Describe/It structure
with Gomega assertions. Preserve all existing scenarios, setup/cleanup behavior,
and expected outcomes while using the package’s established Ginkgo/Gomega
conventions.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: aaf1acff-d56a-4bed-82b0-fe2cb23ad602
📒 Files selected for processing (4)
hack/coverage-changed-files.shtests/e2e/test_rosacli_cluster.gotests/utils/helper/aws.gotests/utils/helper/aws_test.go
…n expired AWS creds Day1-negative OCP-45509 expected an obsolete local subnet-count error; with --yes the CLI reaches OCM and returns cluster_wide_proxy requires subnetIDs. Also skip AWS-heavy day1-negative Describes when STS tokens are expired so infra failures are not recorded as product regressions. Exclude tests/e2e from changed-files coverage so e2e edits do not run the full suite locally. Signed-off-by: Amanda Hager Lopes de Andrade Katz <amanda.katz@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ecac803 to
8177a8a
Compare
| BeforeEach(func() { | ||
| var err error | ||
|
|
||
| helper.SkipIfAWSCredentialsInvalid() |
There was a problem hiding this comment.
I would rather we not add anything to skip on invalid credentials. If we do that and our CI credentials do expire ever, we won't know immediately since all the tests would be skipped and the CI jobs would be marked as passed then. Also, the error I think you're trying to fix here should be fixed already by #3409
| "--prefix", accountRolePrefixToClean, | ||
| "-y") | ||
| Expect(err).To(BeNil()) | ||
| prefix := accountRolePrefixToClean |
There was a problem hiding this comment.
What's the point of this section of code? I don't really see what's being changed other than checking if rosaClient is nil
|
@amandahla: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Moving to draft to investigate better and also considering Jericho's reviews. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
2 participants
PR Summary
Align day1-negative OCP-45509 with current CLI/OCM proxy-without-subnets error, and skip AWS-heavy day1-negative specs when STS credentials are expired so infra noise is not reported as product failures.
Detailed Description of the Issue
periodic-ci-openshift-rosa-master-e2e-rosa-day1-negative-f7has been failing consistently on OCP-45509. The first step callsrosa create cluster --dry-runwith HTTP/HTTPS proxy flags and no--subnet-ids. It still expected the old local messagenumber of subnets for a 'single AZ' 'cluster' should be '2', instead received: '0'.With
--yes(already set byCreateDryRun), the CLI accepts the managed-VPC prompt path and reaches OCM, which rejects the request withcluster_wide_proxy is only supported if subnetIDs exist. That is the real validation for proxy without subnet IDs today, not a missing-VPC flake.Separately, mid-suite STS expiry (
InvalidClientTokenId/ related AWS auth errors) caused rotating product assertion failures on other day1-negative cases.Related Issues and PRs
Type of Change
Previous Behavior
tests/e2esources causedcoverage-changed-filesto attempt running the full e2e suite locally.Behavior After This Change
cluster_wide_proxy is only supported if subnetIDs exist(later VPC/proxy steps unchanged).BeforeEachhooks callhelper.SkipIfAWSCredentialsInvalid()and Skip with an infra message on known credential failures.hack/coverage-changed-files.shexcludestests/e2e/from unit coverage collection.How to Test (Step-by-Step)
Preconditions
make install-hooks).Test Steps
make fmt-checkmake coverage-changed-files(or./hack/coverage-changed-files.shwith staged/committed changes)make rosamake lintmake test GO_TEST_FLAGS='-count=1'go test ./tests/utils/helper/ -count=1 -run 'TestIsAWSCredentialError|TestAWSCredentialsInvalid|TestSkipIfAWSCredentialsInvalid'Expected Results
cluster_wide_proxy is only supported if subnetIDs exist.Proof of the Fix
make fmt-check, coverage (88.9% delta on helper),make rosa,make lint,make test, helper package tests, and CodeRabbit (review --agent --uncommitted) with 0 findings. Pre-push checks passed on push.periodic-ci-openshift-rosa-master-e2e-rosa-day1-negative-f7builds (OCP-45509 consistent; STS failures rotating).Breaking Changes
Breaking Change Details / Migration Plan
N/A
Developer Verification Checklist
[JIRA-TICKET] | [TYPE]: <MESSAGE>.make install-hookshas been run in this clone.make testpasses.make lintpasses.make rosapasses.Made with Cursor
Summary by CodeRabbit
Bug Fixes
Tests
Chores