ROSAENG-60640 | test: Fixing day1-negative test cases#3317
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jerichokeyne 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
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR updates e2e test files for rosacli account roles and cluster tests, plus the cluster_service utility. Changes include adding an Sequence Diagram(s)sequenceDiagram
participant BeforeEach
participant customProfile
participant TempClusterHandler
participant CreateDryRun
BeforeEach->>customProfile: construct HCP+MultiAZ+STS+OIDC+BYO VPC config
BeforeEach->>TempClusterHandler: initialize with customProfile
TempClusterHandler->>CreateDryRun: build create cluster command with --dry-run --mode=auto --yes
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/utils/exec/rosacli/cluster_service.go (1)
247-253: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDrop the extra
-yfromCreateDryRuncall sites that already pass a value-taking flag.CreateDryRunalready prepends--yes, so-yafter--audit-log-arnor the registry-config flags is consumed as that flag’s value, not as a confirm flag. Remove it from those call sites (tests/e2e/test_rosacli_cluster.go:2818, 2861, 2971).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utils/exec/rosacli/cluster_service.go` around lines 247 - 253, The extra -y at the affected CreateDryRun call sites is being parsed as the value for a preceding value-taking flag instead of as a confirmation flag. Update the test call sites that invoke clusterService.CreateDryRun so they no longer append -y after --audit-log-arn or the registry-config flags, since CreateDryRun already injects --yes itself. Use the CreateDryRun helper and the ROSA CLI cluster test cases as the locations to adjust.
🤖 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.
Outside diff comments:
In `@tests/utils/exec/rosacli/cluster_service.go`:
- Around line 247-253: The extra -y at the affected CreateDryRun call sites is
being parsed as the value for a preceding value-taking flag instead of as a
confirmation flag. Update the test call sites that invoke
clusterService.CreateDryRun so they no longer append -y after --audit-log-arn or
the registry-config flags, since CreateDryRun already injects --yes itself. Use
the CreateDryRun helper and the ROSA CLI cluster test cases as the locations to
adjust.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 05cf9d22-2ac4-4622-96e5-c70a3b06f2a2
📒 Files selected for processing (3)
tests/e2e/test_rosacli_account_roles.gotests/e2e/test_rosacli_cluster.gotests/utils/exec/rosacli/cluster_service.go
|
@jerichokeyne: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| rosalCommand.AddFlags("--dry-run", "--external-auth-providers-enabled", "-y") | ||
| } else { | ||
| rosalCommand.AddFlags("--dry-run", "-y") | ||
| rosalCommand.AddFlags("--external-auth-providers-enabled") |
There was a problem hiding this comment.
--dry-run was removed here during refactoring but rosalCommand doesn't include it by default (unlike CreateDryRun). Without it, this runs a real create cluster command. Should be re-added for safety.
| clusterHandler, err = handler.NewTempClusterHandler(rosaClient, customProfile) | ||
| Expect(err).To(BeNil()) | ||
|
|
||
| By("Init the cluster id and testing cluster name") |
There was a problem hiding this comment.
This By has no code between it and the next By -- looks like a leftover from removing the random profile logic.
| }) | ||
| It("create/delete classic account roles with managed policies - [id:57408]", | ||
| labels.Critical, labels.Runtime.OCMResources, | ||
| labels.Critical, labels.Runtime.OCMResources, labels.Exclude, |
There was a problem hiding this comment.
Could you add a brief comment or link to a tracking issue explaining why this critical-labeled test is being excluded?
|
Replying to @coderabbitai's review: Agree with this observation. Lines 2861 and 2971 should drop the Specific call sites (outside the diff, so noted here):
|
✅ Action performedReview finished.
|
2 participants
PR Summary
Fixing day1-negative test failures
Detailed Description of the Issue
Related Issues and PRs
#Type of Change
Previous Behavior
Behavior After This Change
How to Test (Step-by-Step)
Preconditions
Test Steps
Expected Results
Proof of the Fix
Screenshots:
Videos:
Logs/CLI output:
Other artifacts:
Running all
day1-negativetests: https://privatebin.corp.redhat.com/?3ba3df823f17a63a#GvKFJanDjJhd1qLHeARBFDXTW9sfaFVFgGTHDwuzKyLARe-running
id:73755after fixing that test: https://privatebin.corp.redhat.com/?51e973069aee31b8#25zMXYYqrBjp3AfME2mENTqcL9yQgeoHvi8RWJm8TPumBreaking Changes
Breaking Change Details / Migration Plan
Developer Verification Checklist
[JIRA-TICKET] | [TYPE]: <MESSAGE>.make install-hookshas been run in this clone.make testpasses.make lintpasses.make rosapasses.Summary by CodeRabbit
Bug Fixes
Tests