Add serviceRole and instanceProfile to ClusterOptions #159
Conversation
Co-Authored-By: metral <1112768+metral@users.noreply.github.com>
Co-Authored-By: metral <1112768+metral@users.noreply.github.com>
There was a problem hiding this comment.
LGTM. @lukehoban @pgavlin any thoughts?
nit: could you please squash the commit history before we merge? Thanks!
| @@ -216,9 +221,13 @@ export function createCore(name: string, args: ClusterOptions, parent: pulumi.Co | |||
| } else if (args.instanceRole) { | |||
There was a problem hiding this comment.
There is a third branch of this if/else if/else if that also creates an InstanceProfile. I assume we need to update that one as well?
The branching here is a little wild, and probably eventually should get simplified down or refactored a bit - but that can be part of a a follow-up PR.
There was a problem hiding this comment.
You are right, currently the instanceprofile only seems to get created if a single iam role is passed in among the args. Presumably an instanceprofile (or profiles?) need to get get created in the "if" branch as well? In that branch, a list of roles are passed in. The only use case we currently have is when a single role is passed in, so I didn't pay attention to the other logical branches :)
There was a problem hiding this comment.
I think we do need to make the change in the other branch as well to be correct here. We are allowing the user to pass an explicit InstanceRole they want to use, but then in some cases will ignore it and create a new one anyway.
There was a problem hiding this comment.
Ok.. Ill address this shortly. Thanks.
There was a problem hiding this comment.
just pushed a commit for this.
Co-Authored-By: Pat Gavlin <pgavlin@gmail.com>
Co-Authored-By: Pat Gavlin <pgavlin@gmail.com>
|
Thank you guys for the review / comments. Is anything else needed from me at this point to merge this PR? |
There was a problem hiding this comment.
LGTM. Resolve the conflicts and you should be all set. Thanks for the PR!
@lukehoban any thoughts before merging?
Thank you, conflicts resolved! |
|
Just bubbling this up - since I think my note got hidden in a resolved thread:
|
…iform in handling of instanceprofile creation
Addresses issue #159