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: Use a Map internally for workers data structure #1132

Conversation

mariadb-JeffBachtel
Copy link

PR o'clock

Description

This changeset morphs the List structure passed in for workers into a Map, so that it churns less with changes (additions/removals/etc)

This addresses #1105 somewhat (not for all worker types)

Checklist

@mariadb-JeffBachtel mariadb-JeffBachtel changed the title Use a Map internally for workers data structure feat: Use a Map internally for workers data structure Dec 2, 2020
@pre
Copy link

pre commented Dec 4, 2020

I'm in favor of not using an Array ever with Terraform resources, but ..

Wouldn't this be a breaking change - big time ?

Maybe a softer approach for changing an array structure (dangerous terraform) to a map (good terraform), would be to create a separate configuration key altogether. Ie leave the current worker_groups array definition as is and introduce a better one with eg autoscaling_groups (as worker group in this project seems to mean an asg).

Some keys already are implemented using the map format, like node_groups.

(Array is bad because you can't remove an element in the middle of the list without recreating all the resources in that list. Map is good because you can remove elements independently.)

@mariadb-JeffBachtel
Copy link
Author

It's not a breaking change because the array -> map change is done entirely internally, so the input/output interface to the module remains exactly the same. I'm seeing unexpected churn due to launch configuration changes, but I can't verify why.

@mariadb-JeffBachtel
Copy link
Author

I see I have a lint failure, but I'm not able to repeat it locally. What version of terraform is being run for that step?

@mmingorance-dh
Copy link

I'd love to see this PR merged. Is there any ETA when this could be released in a new module version?

@mariadb-JeffBachtel
Copy link
Author

@antonbabenko would you mind approving this for workflow?

@barryib
Copy link
Member

barryib commented May 4, 2021

@mariadb-JeffBachtel thanks for working on this. But since we wanted to split this module into submodule, we want to split this and move from count to for_each in the same time. Probably by using for_each to loop over sub-module.

So this work, is good, but it doesn't go into that direction. I'll update following issues to share a design direction and start a new branch as preview branch.

@barryib barryib added the on hold label May 4, 2021
@mariadb-JeffBachtel
Copy link
Author

I see, thanks Thierno. I don't want to litter the module's PR list, so I'll close for now. Cheers :)

@barryib barryib mentioned this pull request May 5, 2021
2 tasks
@github-actions
Copy link

I'm going to lock this pull request 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 related to this change, 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 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants