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

Feat/bug: workers ASGs that are more resilient to unnecessary change #1105

Closed
3 of 4 tasks
mariadb-JeffBachtel opened this issue Nov 18, 2020 · 16 comments · Fixed by #1680
Closed
3 of 4 tasks

Feat/bug: workers ASGs that are more resilient to unnecessary change #1105

mariadb-JeffBachtel opened this issue Nov 18, 2020 · 16 comments · Fixed by #1680
Milestone

Comments

@mariadb-JeffBachtel
Copy link

I have issues

I'm submitting a...

  • bug report
  • feature request
  • support request - read the FAQ first!
  • kudos, thank you, warm fuzzy

What is the current behavior?

If I mutate the array I pass in for worker_groups at all, all of my ASGs are regenerated and it's kinda explosive

If this is a bug, how to reproduce? Please include a code sample if relevant.

Pass in a list for worker_groups, append an entry on a subsequent run, note the resources to destroy, try not to cry, cry anyway.

What's the expected behavior?

By preference, only ASGs that didn't exist before would be added, or if existing ASGs were changed would they be replaced.

Are you able to fix this problem and submit a PR? Link here if you have already.

Not really but I had an idea that instead of a count loop (https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/workers.tf#L4 ) we could use a for_each loop, maybe. A map would be better for this than a list, so perhaps a different ASG fed by a new module param (worker_groups_map?) would be needed for backwards compatibility. Or maybe you could generate a map from a list I'm no scientist.

Environment details

  • Affected module version: 12.2.0 but the code looks the same in 13.2.1
  • OS: Linux
  • Terraform version: 0.13.4

Any other relevant info

Thanks for this module, I've been using it I think actual years plural now, and it's always super useful and composable (in fact, for this ticket I could easily create my own workers.tf and have it work with your existing module easy peasy)

@pre
Copy link

pre commented Nov 30, 2020

This issue of ”resource needs to be recreated” seems to be a deeper one.

It is also a pain with managed node groups. You can’t modify a launch template without recreating the whole node group, which is just plain wrong: #1109

@mariadb-JeffBachtel
Copy link
Author

I put in a PR that internally creates a map based on the list of workers from the caller (so no API change to module) and uses for_each. That approach could probably be used elsewhere resources are created based on a count

@stale
Copy link

stale bot commented Mar 2, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 2, 2021
@mariadb-JeffBachtel
Copy link
Author

unstale?

@stale stale bot removed the stale label Mar 19, 2021
@james-callahan
Copy link

I would love to see this issue fixed.

Is this module still being maintained? (there hasn't been a commit since January)

@stale
Copy link

stale bot commented Jul 5, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 5, 2021
@james-callahan
Copy link

.

@stale stale bot removed the stale label Jul 7, 2021
@stale
Copy link

stale bot commented Sep 14, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 14, 2021
@james-callahan
Copy link

I think #1309 had a very reasonable solution. It wasn't merged because of upcoming refactoring; but it would be hugely helpful even as an interim solution.

@stale stale bot removed the stale label Sep 15, 2021
@antonbabenko antonbabenko added this to the v18.0.0 milestone Sep 15, 2021
@antonbabenko
Copy link
Member

I put this issue into a backlog for the v18.0.0 milestone because this is a breaking change.

@stale
Copy link

stale bot commented Oct 16, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 16, 2021
@stale
Copy link

stale bot commented Oct 23, 2021

This issue has been automatically closed because it has not had recent activity since being marked as stale.

@stale stale bot closed this as completed Oct 23, 2021
@james-callahan
Copy link

I put this issue into a backlog for the v18.0.0 milestone because this is a breaking change.

If this is part of the milestone then shouldn't this be kept open?

@antonbabenko
Copy link
Member

@james-callahan - Thanks for the report. You are right, I have opened #1656 to track it.

@antonbabenko
Copy link
Member

This issue has been resolved in version 18.0.0 🎉

@github-actions
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants