ROSAENG-60386 | test: Fixing id:84981#3280
Merged
openshift-merge-bot[bot] merged 1 commit intoJun 26, 2026
Merged
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: Enterprise Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR adds AutoNode IAM test resources, helper functions to create and delete IAM roles and policies, structured YAML parsing for 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 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 |
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: 6
🧹 Nitpick comments (1)
tests/utils/config/autonode.go (1)
35-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap returned errors with operation context.
Most AWS/template failures are returned raw, which makes E2E cleanup/setup failures hard to diagnose. Wrap them with the operation and resource name using
%w. As per coding guidelines, “Wrap returned errors with context using%w; do not drop the original error.”🤖 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/config/autonode.go` around lines 35 - 128, Most failures in PrepareAutonodeRoleAndPolicy and DeleteAutonodeRoleAndPolicy are returned directly, which drops useful setup/cleanup context. Update the error returns around CreateAWSClient, helper.ReadFileContent, CreateIAMPolicy/GetIAMPolicy, WaitForResourceExisting, CreateRoleAndAttachPolicy/GetRole, DetachIAMPolicy, DeleteIAMPolicy, and DeleteRole to wrap the original error with the operation and resource name using %w. Keep the existing function names and resource identifiers (policyName, roleName, policyARN, region) in the wrapped messages so E2E failures are easier to trace.Source: Coding guidelines
🤖 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/ci/data/resources/autonode_trust_policy_template.json`:
- Around line 10-13: The trust policy in autonode_trust_policy_template.json is
missing an audience restriction, so it currently trusts the service account
without pinning the token audience. Update the Condition block in the policy
template to require both the existing {OIDC_PROVIDER_URL}:sub match and
{OIDC_PROVIDER_URL}:aud = sts.amazonaws.com, using the same
Condition/StringEquals structure already present in the template.
In `@tests/e2e/hcp_cluster_test.go`:
- Around line 788-791: Reset the runner format before the DescribeCluster
assertion so a failure does not skip cleanup and leave subsequent commands in
JSON mode. In the hcp cluster test around rosaClient.Runner.JsonFormat(),
clusterService.DescribeCluster(clusterID), and rosaClient.Runner.UnsetFormat(),
make sure the runner is restored immediately after the JSON-mode command or via
a deferred restore before Expect(err).To(BeNil()) can abort the test. Keep the
cleanup tied to the same test flow so later commands always run with the default
format.
- Around line 804-807: The deferred AutoNode IAM cleanup in the e2e test is
ignoring returned errors, so failures in DeleteAutonodeRoleAndPolicy go
unnoticed. Update the cleanup around PrepareAutonodeRoleAndPolicy and defer
calls to capture and assert the error from
utilConfig.DeleteAutonodeRoleAndPolicy for both autonodePrefix1 and
autonodePrefix2, so leaked IAM roles/policies fail the test instead of being
silently ignored.
In `@tests/utils/config/autonode.go`:
- Around line 49-101: The AutoNode IAM setup currently leaves partially created
resources behind when later steps fail. Update the CreateAutoNode IAM flow in
autonode.go so that validation for the trust-policy template and OIDC
substitutions happens before creating AWS resources, then add a rollback defer
after successful policy creation and role creation using the existing
awsclient.CreateIAMPolicy, awsclient.CreateRoleAndAttachPolicy, GetIAMPolicy,
and GetRole paths to clean up any created policy/role if a later wait or
substitution step returns an error. Ensure the defer is cleared only once the
function is fully successful so failures during waiting or role setup still
trigger cleanup.
- Around line 115-125: Make autonode cleanup best-effort in the cleanup flow
around DetachIAMPolicy, DeleteIAMPolicy, and DeleteRole: treat expected
NoSuchEntity and “not attached” detach failures as non-fatal, continue
attempting the remaining deletes, and only surface unexpected failures. Update
the cleanup logic in the autonode teardown path to accumulate errors from
awsclient.DetachIAMPolicy, awsclient.DeleteIAMPolicy, and awsclient.DeleteRole,
then return a combined error at the end if any unexpected issues occurred.
In `@tests/utils/exec/rosacli/cluster_service.go`:
- Around line 81-90: AutoNodeDescription.UnmarshalYAML currently ignores
unsupported YAML node kinds and returns nil, which silently leaves AutoNode
zero-valued. Update UnmarshalYAML to fail fast by returning an error for any
value.Kind other than yaml.ScalarNode or yaml.MappingNode, while keeping the
existing Decode behavior for the supported shapes. Use the UnmarshalYAML method
on AutoNodeDescription as the fix point so rosa describe cluster parsing
surfaces invalid AutoNode YAML immediately.
---
Nitpick comments:
In `@tests/utils/config/autonode.go`:
- Around line 35-128: Most failures in PrepareAutonodeRoleAndPolicy and
DeleteAutonodeRoleAndPolicy are returned directly, which drops useful
setup/cleanup context. Update the error returns around CreateAWSClient,
helper.ReadFileContent, CreateIAMPolicy/GetIAMPolicy, WaitForResourceExisting,
CreateRoleAndAttachPolicy/GetRole, DetachIAMPolicy, DeleteIAMPolicy, and
DeleteRole to wrap the original error with the operation and resource name using
%w. Keep the existing function names and resource identifiers (policyName,
roleName, policyARN, region) in the wrapped messages so E2E failures are easier
to trace.
🪄 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: Enterprise
Run ID: 8ec23d47-74d9-4433-af3c-8f4d26e8ae0c
📒 Files selected for processing (5)
tests/ci/data/resources/autonode_policy.jsontests/ci/data/resources/autonode_trust_policy_template.jsontests/e2e/hcp_cluster_test.gotests/utils/config/autonode.gotests/utils/exec/rosacli/cluster_service.go
jerichokeyne
force-pushed
the
ROSAENG-60386
branch
from
054e669 to
52c950e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/utils/config/autonode.go (3)
84-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid shadowing
errin the role reuse path.Line 84 introduces a scoped
errinside thealreadyExistsbranch. Use a named error such asgetRoleErrto keep this helper consistent and avoid accidental use of the wrong error variable during future edits.As per coding guidelines, “Avoid variable shadowing, especially
err,ctx, and AWS or OCM clients.”Suggested change
- roleTemp, err := awsclient.GetRole(roleName) - if err != nil { - return "", err + roleTemp, getRoleErr := awsclient.GetRole(roleName) + if getRoleErr != nil { + return "", getRoleErr }🤖 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/config/autonode.go` around lines 84 - 86, The role reuse path in the helper introduces a shadowed err variable when calling awsclient.GetRole, which should be avoided for consistency and future safety. Rename the scoped error to a distinct name like getRoleErr in the alreadyExists branch, and update the subsequent check and return in the same helper so the original err variable is not shadowed anywhere in that path.Source: Coding guidelines
35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd doc comments for the exported helpers.
PrepareAutonodeRoleAndPolicyandDeleteAutonodeRoleAndPolicyare new exported functions without doc comments, which can trip Go linting and makes the test utility API less clear.As per coding guidelines, “Use exported symbol doc comments when new public types or functions are introduced.”
Also applies to: 104-104
🤖 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/config/autonode.go` at line 35, Add Go doc comments for the new exported helpers in the autonode test utilities so linting passes and the API is clear. Document both PrepareAutonodeRoleAndPolicy and DeleteAutonodeRoleAndPolicy with comments that start with the function name and briefly state what each helper does, keeping the comments adjacent to the function declarations.Source: Coding guidelines
44-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap returned errors with operation context.
Most setup failures return the raw
err, so e2e logs won’t show whether the failure came from AWS client creation, resource file loading, policy creation, role creation, or waiter calls. Wrap these with%wlike the delete path already does.As per coding guidelines, “Wrap returned errors with context using
%w; do not drop the original error.”Suggested pattern
awsclient, err := aws_client.CreateAWSClient("", region) if err != nil { - return "", err + return "", fmt.Errorf("create AWS client for region %s: %w", region, err) }Also applies to: 109-111
🤖 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/config/autonode.go` around lines 44 - 95, Several setup paths in the AutoNode helper return bare errors, so update the error handling in the IAM policy and role creation flow to wrap failures with operation context using %w instead of returning err directly. In the function that creates the AWS client, reads the policy files, calls CreateIAMPolicy, GetIAMPolicy, WaitForResourceExisting, CreateRoleAndAttachPolicy, and GetRole, make each returned error include a short message that identifies the failing step while preserving the original error.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@tests/utils/config/autonode.go`:
- Around line 84-86: The role reuse path in the helper introduces a shadowed err
variable when calling awsclient.GetRole, which should be avoided for consistency
and future safety. Rename the scoped error to a distinct name like getRoleErr in
the alreadyExists branch, and update the subsequent check and return in the same
helper so the original err variable is not shadowed anywhere in that path.
- Line 35: Add Go doc comments for the new exported helpers in the autonode test
utilities so linting passes and the API is clear. Document both
PrepareAutonodeRoleAndPolicy and DeleteAutonodeRoleAndPolicy with comments that
start with the function name and briefly state what each helper does, keeping
the comments adjacent to the function declarations.
- Around line 44-95: Several setup paths in the AutoNode helper return bare
errors, so update the error handling in the IAM policy and role creation flow to
wrap failures with operation context using %w instead of returning err directly.
In the function that creates the AWS client, reads the policy files, calls
CreateIAMPolicy, GetIAMPolicy, WaitForResourceExisting,
CreateRoleAndAttachPolicy, and GetRole, make each returned error include a short
message that identifies the failing step while preserving the original error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 10306a23-9051-485c-bee5-25df27ac968d
📒 Files selected for processing (5)
tests/ci/data/resources/autonode_policy.jsontests/ci/data/resources/autonode_trust_policy_template.jsontests/e2e/hcp_cluster_test.gotests/utils/config/autonode.gotests/utils/exec/rosacli/cluster_service.go
✅ Files skipped from review due to trivial changes (1)
- tests/ci/data/resources/autonode_trust_policy_template.json
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/utils/exec/rosacli/cluster_service.go
- tests/ci/data/resources/autonode_policy.json
- tests/e2e/hcp_cluster_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3280 +/- ##
=======================================
Coverage 26.17% 26.17%
=======================================
Files 334 334
Lines 36704 36704
=======================================
Hits 9608 9608
Misses 26359 26359
Partials 737 737 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
amandahla
reviewed
Jun 25, 2026
amandahla
reviewed
Jun 25, 2026
amandahla
reviewed
Jun 25, 2026
amandahla
reviewed
Jun 25, 2026
Contributor
|
The PR description has Shouldnt show 1 Passed? |
amandahla
reviewed
Jun 25, 2026
Contributor
Author
Whoops. I guess I accidentally included the text for a second test run, and not the first test run. (I added code to skip if autonode is already enabled since you can't disable it yet). I updated the description now |
jerichokeyne
force-pushed
the
ROSAENG-60386
branch
from
52c950e to
9af185c
Compare
Contributor
Author
|
@amandahla I just pushed some changes to address some of your comments. Please take another look when you get a chance |
jerichokeyne
force-pushed
the
ROSAENG-60386
branch
from
9af185c to
511f38d
Compare
Contributor
|
/approve |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amandahla, 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-merge-bot
Bot
merged commit 80fd9fb
into
openshift:master
11 of 13 checks passed
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
2 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.
PR Summary
Fixes id:84981 by creating properly configured IAM roles and policies for autonode support
Detailed Description of the Issue
Related Issues and PRs
#Type of Change
Previous Behavior
id:84981 would always fail, and describing a cluster (which is done during the cleanup phase) would also fail to parse the output when autonode is properly enabled
Behavior After This Change
id:84891 creates 2 IAM roles and policies and passes. You can also delete a cluster after running this test since the code can properly parse the autonode description now
How to Test (Step-by-Step)
Preconditions
Test Steps
Expected Results
Proof of the Fix
test case passing
running the test case again
Can delete cluster
Breaking 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
Summary by CodeRabbit