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

Make node_pools_* params optional and allow pool-specific overrides. #318

Merged

Conversation

cray0000
Copy link
Contributor

@cray0000 cray0000 commented Nov 13, 2019

The main idea of the PR is to move setting the dynamic default values for node_pools_* options into a separate step:

  • node_pools_labels
  • node_pools_metadata
  • node_pools_taints
  • node_pools_tags
  • node_pools_oauth_scopes

Therefore I added a new file autogen/variables_defaults.tf the purpose of which is to setup the dynamic defaults for variables and shadow them with the locals of the same name.

I've also changed the example/test case node_pools -- it selectively sets node_pools_* options only for some pools. Also note the complete absense of the node_pools_oauth_scopes option in the example.

@cray0000
Copy link
Contributor Author

The decision to make setting the defaults values a separate logical step was made instead of the implementation seen in #269 because that implementation was leading to multiple hardcoded patches in the code logic.

@cray0000 cray0000 self-assigned this Nov 13, 2019
@cray0000 cray0000 added the enhancement New feature or request label Nov 13, 2019
@aaron-lane aaron-lane self-assigned this Nov 14, 2019
Copy link
Contributor

@aaron-lane aaron-lane left a comment

Choose a reason for hiding this comment

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

Thanks @cray0000!

Makefile Outdated
@@ -18,7 +18,7 @@
# Make will use bash instead of sh
SHELL := /usr/bin/env bash

DOCKER_TAG_VERSION_DEVELOPER_TOOLS := 0.4.6
DOCKER_TAG_VERSION_DEVELOPER_TOOLS := 0.5.4
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
DOCKER_TAG_VERSION_DEVELOPER_TOOLS := 0.5.4
DOCKER_TAG_VERSION_DEVELOPER_TOOLS := 0

build/int.cloudbuild.yaml Outdated Show resolved Hide resolved
build/lint.cloudbuild.yaml Outdated Show resolved Hide resolved
@aaron-lane
Copy link
Contributor

I may have messed up the merge commit based on the lint result. 😊

@cray0000 cray0000 requested a review from aaron-lane November 19, 2019 16:32
aaron-lane
aaron-lane previously approved these changes Nov 20, 2019
Copy link
Contributor

@aaron-lane aaron-lane left a comment

Choose a reason for hiding this comment

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

@cray0000 please rebase against master and we can merge this!

[test/deploy_service] Add retry logic to wait until the nginx service becomes reachable

[build] Add 120 seconds delay after the 'prepare' step
@cray0000 cray0000 force-pushed the f-make-params-optional branch from 1a1d449 to bf48602 Compare November 20, 2019 17:00
@cray0000
Copy link
Contributor Author

@aaron-lane In the last run converge sandbox-enabled-local seems to take a lot of time to provision for no apparent reason. And timed out in 30 mins.
I think it should be safe to merge, thanks!

@aaron-lane aaron-lane merged commit fbc2604 into terraform-google-modules:master Nov 21, 2019
CPL-markus pushed a commit to WALTER-GROUP/terraform-google-kubernetes-engine that referenced this pull request Jul 15, 2024
…params-optional

Make node_pools_* params optional and allow pool-specific overrides.
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
2 participants