Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OCM-8256 | feat: add methods to attach policies #49

Closed
wants to merge 2 commits into from

Conversation

OAharoni-RedHat
Copy link
Contributor

1 new method: CreateRoleAndAttachPolicy

other added methods are migrated from openshift-rosa-cli: https://github.com/OAharoni-RedHat/openshift-rosa-cli/blob/master/pkg/aws_client/aws_v1/role.go

@yingzhanredhat
Copy link
Contributor

@OAharoni-RedHat Do you remove completeRolePolicyDocument function?

@OAharoni-RedHat
Copy link
Contributor Author

@yingzhanredhat I'm not, as far as I can tell it's still required for how the formatting of the new CreateRole function works. Am I mistaken there?

@yingzhanredhat
Copy link
Contributor

/lgtm

path string,
policyArn string) (types.Role, error) {
role, err := client.CreateRole(roleName, assumeRolePolicyDocument, permissionBoundry, tags, path)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OAharoni-RedHat Do you need to check the err before you try and call client.AttachPolicy? For example: what if err is a failure to create the role? We should return a clear error here if the role can not be created?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Rob, good catch, I miswrote this statement. I meant to only attempt to attach the policy if there's no error rather than only if there's an error! I think that should cover what you're suggesting here with clear error returns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants