[SREP-644]: added check logic to assume-initial-arn#691
[SREP-644]: added check logic to assume-initial-arn#691openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #691 +/- ##
==========================================
+ Coverage 50.95% 51.15% +0.20%
==========================================
Files 76 76
Lines 5764 5782 +18
==========================================
+ Hits 2937 2958 +21
+ Misses 2422 2420 -2
+ Partials 405 404 -1
🚀 New features to boost your workflow:
|
9286da2 to
ca26766
Compare
|
is there a use case for us to keep this configurable in backplane cli config? |
cmd/ocm-backplane/cloud/common.go
Outdated
| } | ||
|
|
||
| logger.Debugf("set assume-initial-arn to: %s", cfg.BackplaneConfiguration.AssumeInitialArn) | ||
| } else if !slices.Contains([]string{ |
There was a problem hiding this comment.
Minor nit.. The following may improve the readability. Otherwise, it's hard to find where elseif ends
} else if !slices.Contains([]string
{
productionAssumeInitialArn,
stagingAssumeInitialArn,
integrationAssumeInitialArn,
}, cfg.BackplaneConfiguration.AssumeInitialArn)
{
............
}
There was a problem hiding this comment.
I was able to improve readability as below.
else if !slices.Contains(
[]string{
productionAssumeInitialArn,
stagingAssumeInitialArn,
integrationAssumeInitialArn,
},
cfg.BackplaneConfiguration.AssumeInitialArn,
) {
logger.Warnf("assume-initial-arn in backplane config is not set to a valid payer ARN, using: %s", cfg.BackplaneConfiguration.AssumeInitialArn)
return aws.Credentials{}, fmt.Errorf("invalid assume-initial-arn: %s, must be one of: prod: %s, stage: %s, int: %s",
cfg.BackplaneConfiguration.AssumeInitialArn,
productionAssumeInitialArn,
stagingAssumeInitialArn,
integrationAssumeInitialArn,
)
}
I didn't work as suggested due to !slices.Contains( []string{ wants its opening brace { on the same line
There was a problem hiding this comment.
Thanks for improving the readability @diakovnec , suggested solution is just an example of how elseif mixed with } brace
|
I have the same feeling as @bmeng , this configuration may be able to be deleted entirely. Users should not be allowed to add random ARN's in backplane configs.. |
ca26766 to
5c77774
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: diakovnec, samanthajayasinghe 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 |
|
@diakovnec: 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. |
What type of PR is this?
What this PR does / Why we need it?
This addresses a bug identified and discussed in https://redhat-internal.slack.com/archives/CCX9DB894/p1740008344309409?thread_ts=1739912990.153199&cid=CCX9DB894
bp-cli's initial checking logic here, it only sets the assume-initial-arn if it is empty (""), if user manually set an incorrect ARN, it's not empty, then, code skips the override logic and using the incorrect value.
Added logic for checking the correct ARN and returning a clear error message if the wrong ARN has been provided.
example: