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

Updated node_pools variable documentation #297

Closed
wants to merge 11 commits into from

Conversation

neelesh9795
Copy link
Contributor

Added the wrong file last time. Updated the autogen/README.md

Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the formatting of this (you can preview on GitHub). I'd suggest formatting as a table.

Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please recheck requirements and provide defaults from the codebase.

| Name | Description | Requirement |
| --- | --- | --- |
| name | The name of the node pool. If left blank, Terraform will auto-generate a unique name | Optional |
| min_count | Minimum number of nodes in the NodePool. Must be >=0 and <= max_count | Required |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

| --- | --- | --- |
| name | The name of the node pool. If left blank, Terraform will auto-generate a unique name | Optional |
| min_count | Minimum number of nodes in the NodePool. Must be >=0 and <= max_count | Required |
| max_count | Maximum number of nodes in the NodePool. Must be >= min_count | Required |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not required.

Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a column explaining defaults.

@aaron-lane aaron-lane added the enhancement New feature or request label Nov 4, 2019
@aaron-lane
Copy link
Contributor

This should incorporate the addition of node_locations in #303.

@thecodejunkie
Copy link

Does auto_upgrade actuall have a default value of false?

I just tried this module with the following node pool definition

{
    autoscaling        = false
    name               = var.default_node_pool.name
    machine_type       = var.default_node_pool.machine_type
}

after I ran terraform plan I could see the following in the output for the node pool

+management {
  + auto_repair  = true
  + auto_upgrade = true
}

but if I explicitly add auto_upgrade = false then I correctly displayed as

+ management {
   + auto_repair  = true
   + auto_upgrade = false
}

@morgante
Copy link
Contributor

morgante commented Nov 6, 2019

The default depends on whether the cluster is regional or zonal: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/master/autogen/main.tf#L62

@thecodejunkie
Copy link

@morgante ah! Then I humble request that default values, that depend on whether the cluster is regional or zonal, are also reflected in this part of the documentation. Keep up the awesome work!

@morgante
Copy link
Contributor

morgante commented Nov 6, 2019

Agreed. @neelesh9795 please make sure to update with that.

@thecodejunkie
Copy link

@neelesh9795 please also consider sorting the table in alphabetical order 🤗

@@ -182,6 +182,26 @@ In order to operate with the Service Account you must activate the following API
- Compute Engine API - compute.googleapis.com
- Kubernetes Engine API - container.googleapis.com

## node_pools variable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This table must include node_count which was added in #313.

@neelesh9795
Copy link
Contributor Author

Closing this due to merge conflicts. Re opening at #331

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants