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

Do not create instance role when skipDefaultNodeGroup is enabled #1411

Merged

Conversation

flostadler
Copy link
Contributor

@flostadler flostadler commented Sep 30, 2024

When enabling skipDefaultNodeGroup we should not be creating resources related to the default node group. This includes the default instance role.

With this change the Cluster component will no longer create this default role when skipDefaultNodeGroup is enabled.

One of the tests needs re-recording because we're introducing a breaking change (i.e. not creating the extraneous instance role), but in doing so we're aligning the provider with user expectations. Once this is merged in I'll release the next version and re-record.

Fixes #1176

@flostadler flostadler added the needs-release/major Marking a PR to compute the next major version label Sep 30, 2024
@flostadler flostadler self-assigned this Sep 30, 2024
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@flostadler flostadler marked this pull request as draft September 30, 2024 17:49
@flostadler flostadler marked this pull request as ready for review October 3, 2024 17:02
@flostadler flostadler requested review from t0yv0, corymhall and a team October 3, 2024 17:46
@@ -289,7 +289,7 @@ class ClusterNodeGroupOptionsArgsDict(TypedDict):

Note: Given the inheritance of auto-generated CF tags and `cloudFormationTags`, you should either supply the tag in `autoScalingGroupTags` or `cloudFormationTags`, but not both.
"""
bootstrap_extra_args: NotRequired[str]
bootstrap_extra_args: NotRequired[pulumi.Input[str]]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merging #1414 & #1415 ontop of each other dropped those changes.
This adds is back now, work tree is clean again

Copy link
Member

Choose a reason for hiding this comment

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

Are we missing CI check guarantees that the worktree should be clean possibly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're merging those into a feature branch that's only built when cutting an alpha release or when triggered manually. It's checking it as part of those workflows


### Cluster does not create extraneous instance IAM role if `skipDefaultNodeGroup` is set to `true`

Previously the Cluster component created a default instance IAM role even if `skipDefaultNodeGroup` was set to `true`. This role gets correctly omitted now if you're specifying `skipDefaultNodeGroup`.
Copy link
Member

Choose a reason for hiding this comment

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

"if you are"

@flostadler flostadler merged commit 734e88a into release-3.x.x Oct 4, 2024
34 checks passed
@flostadler flostadler deleted the flostadler/no-role-with-skip-default-node-group branch October 4, 2024 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-release/major Marking a PR to compute the next major version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants