Add roleName option #82
Conversation
There was a problem hiding this comment.
@holidayworking Thank you for your great PR!👍 I've left a few comments that should be addressed before this gets merged.😸
aws/saml.go
Outdated
| if roleName != "" && !strings.HasSuffix(roleArn, roleName) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
HasSuffix can cause a problem. For example, when there are two roles "SuperTestRole" and "TestRole" and "TestRole" is specified as roleName, strings.HasSuffix("SuperTestRole", "TestRole") can return true. This is not expected behavior.
I suggest to split roleArn by "/" and compare the last substring with roleName.
| if roleName != "" && !strings.HasSuffix(roleArn, roleName) { | |
| continue | |
| } | |
| splitStrings := strings.Split(roleArn, "/") | |
| attrRoleName := splitStrings[len(splitStrings) - 1] | |
| if roleName != "" && attrRoleName != roleName { | |
| continue | |
| } |
There was a problem hiding this comment.
I overlooked that.
Fixed 8d256af.
aws/saml_test.go
Outdated
| }, | ||
| wantRoleArn: "arn:aws:iam::012345678901:role/TestRole", | ||
| wantPrincipalArn: "arn:aws:iam::012345678901:saml-provider/TestProvider", | ||
| }, | ||
| { | ||
| name: "returns first role when role role attribute are multi and no roleName argument", |
There was a problem hiding this comment.
Please fix "role role" to "role".
There was a problem hiding this comment.
Fixed 047604d.
aws/saml_test.go
Outdated
| wantPrincipalArn: "arn:aws:iam::012345678901:saml-provider/TestProvider1", | ||
| }, | ||
| { | ||
| name: "returns specify role when role role attribute are multi and roleName argument", |
There was a problem hiding this comment.
Fixed 047604d.
|
@miyajan |
There was a problem hiding this comment.
@holidayworking
Thank you for your contribution!
This looks good.
Can you squash your commits into meaningful units?
In this case, I think one is enough.
holidayworking
force-pushed
the
add_role_name
branch
from
047604d to
c9c0a6b
Compare
I squashed my commits. |
|
@holidayworking v1.2.0 is released! Please try it. |
|
@ganta |
assam is great tools. But this tool cannot be used in my environment.
I have two IAM roles as below.
I want to use TestRole2, but this tool uses TestRole1.
I can choose the role I want to use.