Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add serviceRole and instanceProfile to ClusterOptions #159
Add serviceRole and instanceProfile to ClusterOptions #159
Changes from 3 commits
82848f1
3b514be
ef8fd6c
67288ea
b9127f0
2495e4b
a0c61ab
d060515
10c7079
01da3f1
43248e5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a third branch of this
if/else if/else if
that also creates anInstanceProfile
. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.. Ill address this shortly. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just pushed a commit for this.