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

Adapting for use with latest providers #144

Conversation

brandonjbjelland
Copy link

@brandonjbjelland brandonjbjelland commented Apr 24, 2019

What's in this change?

  1. The core fix was removing the node_pool inline block in favor of the now supported initial_node_count.
  2. resources and data lookups now use location instead of zone or region to get remove deprecation warnings.
  3. aligning helper script combine_docfiles.py with upstream.

Why make the change?

The motivation here started with being unable to use the module as is at the v2.0.0 release. The node-pool inline block was the source of this error:

Error: Error applying plan:

1 error(s) occurred:

* module.gke.google_container_cluster.primary: 1 error(s) occurred:

* google_container_cluster.primary: googleapi: Error 400: Cluster.initial_node_count must be greater than zero., badRequest

Replacing zone and region with location was motivated by wanting to resolve deprecation warnings:

Warning: module.gke.data.google_container_engine_versions.region: "region": [DEPRECATED] Use location instead
Warning: module.gke.data.google_container_engine_versions.zone: "zone": [DEPRECATED] Use location instead
Warning: module.gke.google_container_cluster.primary: "region": [DEPRECATED] Use location instead
Warning: module.gke.google_container_node_pool.pools: "region": [DEPRECATED] use location instead

Note: With the new location attribute available, it's possible (though a breaking change) we could also move both regional and zonal clusters and pools to be the same resources rather than spread them across two flavors. I didn't do that here because it's rather impactful but potentially a direction to take this next in simplifying.

Fixes #143

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.

Thanks for the contribution! LGTM, but can you verify whether this is a breaking change for existing clusters?

autogen/cluster_regional.tf Show resolved Hide resolved
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.

Looks like upgrades are breaking and CI is failing.

…tial_node_count into zonal clusters to align with regional
@brandonjbjelland brandonjbjelland force-pushed the master branch 10 times, most recently from 2ce8e10 to 190bff9 Compare April 25, 2019 18:51
@brandonjbjelland brandonjbjelland force-pushed the master branch 2 times, most recently from c11d2cd to b6f6bad Compare April 25, 2019 20:47
@brandonjbjelland brandonjbjelland force-pushed the master branch 3 times, most recently from 01b9516 to 2604a96 Compare April 26, 2019 05:58
@brandonjbjelland
Copy link
Author

We're in a conflicting state here and since my current client is not looking to even use this module, it's exceedingly difficult to find the time to follow through. If that decision changes (through pain) or some time clears up, I'll revisit this work.

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

Successfully merging this pull request may close these issues.

Terraform Google provider >= 2.4 throws error: "node_pool": conflicts with remove_default_node_pool
2 participants