-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(TPG>=7.0)!: adding default_compute_class_enabled #2434
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
Conversation
Thanks @rs1986x - Can you also please bump the minimum TPG constraint as appropriate, that will make the version constraint failure clear rather than unknown Also, https://github.com/terraform-google-modules/terraform-google-project-factory/releases/tag/v18.1.0, now includes support for TPG v7. :) |
Hi @apeabody thanks for the comment, is there anything else you would require before merging the PR? thanks |
/gcbrun |
Probably wait for v40 for TPG>=7 |
No problem. For now i am using my fork, i imagine that eventually somebody would have made the same change i have made but if it can help speed up the release why not. Let me know if you require any other change, in the meantime thanks again and i look forward to see the default class parameter in the main modules |
/gcbrun |
At a minimum this PR is needed first: #2446 |
/gcbrun |
Currently waiting on terraform-google-modules/terraform-google-bastion-host#231 |
/gcbrun |
/gcbrun |
/gcbrun |
/gemini review |
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.
Code Review
This pull request introduces the default_compute_class_enabled
parameter, which requires bumping the Google provider version to >=7.0.0
. The changes are consistently applied across the main module and its submodules. I've identified a potential issue in non-autopilot clusters where default_compute_class_enabled
is set unconditionally, which could lead to errors if autoscaling is disabled. I've provided suggestions to make this setting conditional for improved robustness. The rest of the changes, including documentation and metadata updates, look good.
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.
Thanks for the contribution @rs1986x!
variable "default_compute_class_enabled" { | ||
type = bool | ||
description = "Enable Spot VMs as the default compute class for Node Auto-Provisioning" | ||
default = null | ||
} | ||
|
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.
description is misleading, no? I believe this has nothing to do with SPOT vms?
Hi
i have added the default_compute_class_enabled parameter. it is supported since version 7 of the provider so it's all good. the test fail in some examples because other dependency modules (e.g. project factory) have not yet updated to version 7 and for that reason it uses still provider 6.x