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: Make auto_provisioning_defaults a non-beta feature #1077

Merged
merged 1 commit into from Nov 30, 2021

Conversation

@stanley98yu
Copy link
Contributor

@stanley98yu stanley98yu commented Nov 29, 2021

Fixes #1008

This PR simply makes the auto-provisioning defaults block not beta-exclusive, but I also want to add the min_cpu_platform argument in the block as specified here since it seems we added the feature for default node pools in #1057. However, I don't understand why we pull the value from the first node pool using lookup(var.node_pools[0],...) because, from what I understand, min_cpu_platform is a cluster-level variable, so it feels weird to read from a specific node pool's arguments, like pool-01, even though it will apply to other node pools as well. Shouldn't it be its own variable and read in like var.min_cpu_platform or something similar?

@stanley98yu stanley98yu changed the title Make auto_provisioning_defaults a non-beta feature feat: Make auto_provisioning_defaults a non-beta feature Nov 29, 2021
@morgante
Copy link
Contributor

@morgante morgante commented Nov 29, 2021

Shouldn't it be its own variable and read in like var.min_cpu_platform or something similar?

It's actually not a cluster-level variable. min_cpu_platform is part of the node pool config for particular nodes.

The cluster-level setting is for node pool autoprovisioning. ie. what setting to use for any autoprovisioned node pools. We've made the decision to have your first node pool be your "default" node pool and the settings from it are mirrored to any autoprovisioned node pools.

@comment-bot-dev
Copy link

@comment-bot-dev comment-bot-dev commented Nov 29, 2021

Thanks for the PR! 🚀
Lint checks have passed.

@stanley98yu
Copy link
Contributor Author

@stanley98yu stanley98yu commented Nov 30, 2021

That makes sense. Thanks for the explanation! The min_cpu_platform for auto_provisioning_defaults mirror the first node pool's value as expected then.

@stanley98yu stanley98yu marked this pull request as ready for review Nov 30, 2021
@morgante morgante merged commit 5603718 into master Nov 30, 2021
3 checks passed
@morgante morgante deleted the feat/autoscaling branch Nov 30, 2021
bharathkkb added a commit that referenced this issue Jan 21, 2022
…_cpu_platform` for auto-provisioned node pools (#1077)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants