OCM-23781 | fix: Adding better logic for setting rosa_region tag#3235
OCM-23781 | fix: Adding better logic for setting rosa_region tag#3235jerichokeyne wants to merge 1 commit into
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[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 |
📝 WalkthroughWalkthroughThis pull request introduces FedRAMP-aware region selection throughout the codebase. A new constant 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/ocm/regions.go`:
- Around line 100-101: In pkg/ocm/regions.go replace fmt.Errorf calls that
format wrapped errors with %v or %s to use %w so callers can use errors.Is/As;
specifically update the error returns whose messages start with "error creating
AWS client", and the other fmt.Errorf occurrences in the same file (the ones
that currently interpolate err with %v/%s around the AWS client and related
error messages) to use %w and pass the original err as the last argument,
leaving the rest of the message intact.
🪄 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: df62408d-e907-41c1-a4b7-206a9956c84d
📒 Files selected for processing (5)
pkg/aws/client.gopkg/aws/cloudformation.gopkg/ocm/regions.gotests/e2e/test_rosacli_cluster.gotests/e2e/test_rosacli_region.go
| return nil, fmt.Errorf("error creating AWS client: %v", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify remaining non-wrapping fmt.Errorf(err) patterns in this file.
rg -nP --type=go 'fmt\.Errorf\(".*%(v|s)".*,\s*err\)' pkg/ocm/regions.goRepository: openshift/rosa
Length of output: 375
Use %w for error formatting to enable errors.Is/As unwrapping.
Lines 100, 107, 117, and 155 use %v/%s with error values, preventing callers from using errors.Is/As to check the underlying error types. Change these to %w for proper error chain handling.
Suggested patch
- return nil, fmt.Errorf("error creating AWS client: %v", err)
+ return nil, fmt.Errorf("error creating AWS client: %w", err)
@@
- return nil, fmt.Errorf("failed to get local AWS credentials: %v", err)
+ return nil, fmt.Errorf("failed to get local AWS credentials: %w", err)
@@
- return nil, fmt.Errorf("failed to build AWS credentials for user '%s': %v", aws.AdminUserName, err)
+ return nil, fmt.Errorf("failed to build AWS credentials for user '%s': %w", aws.AdminUserName, err)
@@
- err = fmt.Errorf("failed to retrieve AWS regions: %s", err)
+ err = fmt.Errorf("failed to retrieve AWS regions: %w", err)📝 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.
| return nil, fmt.Errorf("error creating AWS client: %v", err) | |
| } | |
| return nil, fmt.Errorf("error creating AWS client: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/ocm/regions.go` around lines 100 - 101, In pkg/ocm/regions.go replace
fmt.Errorf calls that format wrapped errors with %v or %s to use %w so callers
can use errors.Is/As; specifically update the error returns whose messages start
with "error creating AWS client", and the other fmt.Errorf occurrences in the
same file (the ones that currently interpolate err with %v/%s around the AWS
client and related error messages) to use %w and pass the original err as the
last argument, leaving the rest of the message intact.
1 participant
PR Summary
Updating the logic when creating the osdCcsAdmin user to always set the same value for the
rosa_regiontag regardless of whether or not the user already exists. Also, adding a bit of logic to change the value if running in FedRAMP and the value is set to a non-Govcloud regionThere's also some included changes that were necessary to fix linting issues since I'm updating files that haven't been touched in a while
Detailed Description of the Issue
There's an issue where if the user was running in FedRAMP, and already had a
osdCcsAdminuser created, but that user was missing therosa_regiontag, then when they ranrosa initit would set therosa_regiontag tous-east-1. That would then prevent the user from creating clusters as we default to running in that region if that user and tag exist.Slack threads:
Related Issues and PRs
Type of Change
Previous Behavior
When the user ran
rosa initand created theosdCcsAdminuser it was a bit of a toss up as to what therosa_regionvalue would be set to. If the user already existed, but did not have that tag set, then it would always be set tous-east-1. If the user already existed, and the tag was set, then it doesn't update the tag. If the user did not already exist, then it would be set to whatever value the user passed as--regionBehavior After This Change
Now things have been more unified. In either case of when the
rosa_regiontag needs to be updated, it will be set to the same value. The new way to determine the value is:--region, then use that valueus-gov-east-1us-east-1I also added a check for if the user is in FedRAMP, and for some reason the tag is set to the old default of
us-east-1, then it will update the tag to the new value as specified aboveIf the user and tag already exist, then there is still no change (except for the previous exception)
How to Test (Step-by-Step)
Preconditions
Test Steps
osdCcsAdminuser in the AWS accountrosa init, and check the createdosdCcsAdminand therosa_regiontag on the user. It should be set tous-east-1rosa init --region us-west-2, and check that therosa_regiontag hasn't been updatedrosa_regiontagrosa init --region us-west-2, and check that therosa_regiontag has been set tous-west-2us-gov-east-1andus-gov-west-1rosa_regiontag to be set tous-east-1rosa init --region us-gov-west-1, and check that therosa_regiontag has been set tous-gov-west-1Proof of the Fix
Running the modified test cases locally
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
New Features
Improvements