feat: allow cluster creation without node pool#2520
feat: allow cluster creation without node pool#2520apeabody merged 12 commits intoterraform-google-modules:mainfrom
Conversation
This new example demonstrates creating a regional private cluster without node pool creation and with the default compute class enabled.
Summary of ChangesHello @m0ps, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the GKE Terraform module by allowing users to provision a GKE cluster without requiring an initial node pool definition. This change provides greater flexibility in cluster setup, particularly for scenarios where node pools might be managed separately or added post-creation. The implementation involves conditional logic to handle the absence of node pool configurations and is validated with a new dedicated example. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature to allow GKE cluster creation without any initial node pools. The implementation is solid, using a local.head_node_pool to gracefully handle cases where var.node_pools is empty. This change is consistently applied across the module's templates and generated files. The logic for deriving the cluster name has also been refactored to be more robust by not depending on the presence of node pools. A new example and corresponding integration tests have been added to validate this new capability. My review includes a couple of suggestions for the new example to ensure its correctness and consistency with the rest of the codebase.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature to allow the creation of GKE clusters without any predefined node pools. The changes are well-implemented across the various modules and templates.
Key changes include:
- A new local variable
local.head_node_poolis introduced to safely handle an emptyvar.node_poolslist, preventing errors when a cluster is created without node pools. - The logic for deriving the cluster name (
cluster_name_computed) has been refactored to rely on the cluster ID instead of a node pool ID, making it more robust. - Dependencies on node pool resources in outputs have been correctly removed.
- A new example
simple_regional_private_no_poolhas been added, along with a corresponding integration test, which effectively demonstrates and validates the new functionality.
The code is clean, consistent, and the changes are applied correctly throughout the repository. The version bumps across many examples are noted but seem appropriate for this feature release. Overall, this is a solid contribution that improves the module's flexibility.
|
Here is another report of the same issue: #2303 |
|
/gcbrun |
|
Could you please share details of the failed job @apeabody ? |
|
Thanks @m0ps - This is just a text fixture mismatch, let me re-run. We might need to skip validating the |
|
Ah… when I tested it in my local env, one of ~10 attempts failed with a similar issue. 🤷♂️ |
|
Hm... Still failing... same issue, @apeabody ? |
|
Yes @m0ps I suggest pulling the |
|
Thanks for the hint, @apeabody. It absolutely makes sense... This test creates a cluster without a node pool, similar to autopilot clusters, and the test for autopilot also accepts both "RUNNING" and "RECONCILING" states. I've pushed the change. Additionally, I noticed that one of the test clusters is prevented from being deleted because the |
|
/gcbrun |
|
All 4 checks are green now. Please let me know if something else is required, @apeabody. If not, I hope it will be ok to merge. |
|
Thanks for the question, @apeabody. I forgot to clarify this important point: this change doesn't affect existing clusters. Frankly, I haven't tested all examples with this change; I've only tested about 5 randomly selected ones. The nature of the change is pretty simple: The only caveat is that if someone wants to create a brand-new cluster without a nodepool, the default compute class should be enabled: Heh... you know... I think that what has been mentioned in the previous paragraph is exactly what you are looking for. I guess it makes sense to mention the following:
|
|
So... any feedback, @apeabody? |
8b65cd7
into
terraform-google-modules:main
#2408 seems stuck, so I created this MR to allow creation of GKE clusters without node pools defined. This will help to resolve #2407 (which has been closed as non-planned due to the lack of attention).
I've also created a test case (which leverages
simple_regional_privatesubmodule) to validate functionality.Closes: #2408
Fixes: #2407