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

Update to support kubernetes provider v2 #93

Merged
merged 8 commits into from
Jan 22, 2021
Merged

Conversation

woz5999
Copy link
Contributor

@woz5999 woz5999 commented Jan 22, 2021

Breaking Changes

  • This module requires kubernetes Terraform provider >= 2.0, which introduced breaking changes to the provider. This module has made additional breaking changes in response.
  • Inputkubernetes_load_config_file has been removed because the corresponding load_config_file option to the Kubernetes provider has been removed in provider version 2.0.
  • Input kubernetes_config_path has been removed as its use case (Allow k8s config file load #77, feat: add k8s provider config variables #78) was for one-time migration and in general we want to discourage its use. If needed anyway, a value can be provided via the KUBE_CONFIG_PATHS environment variable. If a more compelling use case is made, it will be a non-breaking change to restore it.
  • Users of previous versions of this module can continue to use those versions by pinning the Kubernetes provider version in their root module. For Terraform >= 0.12.26, add this block to your required_providers block
    kubernetes = {
      source  = "hashicorp/kubernetes"
      version = "~> 1.11"
    }

what

  • Update to support kubernetes provider v2

why

  • Better. Stronger. Faster.

references

@woz5999 woz5999 requested review from a team as code owners January 22, 2021 00:42
@@ -355,8 +355,8 @@ Available targets:
| environment | Environment, e.g. 'uw2', 'us-west-2', OR 'prod', 'staging', 'dev', 'UAT' | `string` | `null` | no |
| id\_length\_limit | Limit `id` to this many characters.<br>Set to `0` for unlimited length.<br>Set to `null` for default, which is `0`.<br>Does not affect `id_full`. | `number` | `null` | no |
| kubernetes\_config\_map\_ignore\_role\_changes | Set to `true` to ignore IAM role changes in the Kubernetes Auth ConfigMap | `bool` | `true` | no |
| kubernetes\_config\_path | Path to the kube config file. Defaults to `~/.kube/config` | `string` | `"~/.kube/config"` | no |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to provider behavior changes, this value no longer has a default. Previously, if load_config_file, was false, this config was ignored. Updating the default to match the provider's maintains the same behavior as before.

@woz5999
Copy link
Contributor Author

woz5999 commented Jan 22, 2021

/test all

@woz5999 woz5999 requested a review from aknysh January 22, 2021 00:51
@vsimon
Copy link

vsimon commented Jan 22, 2021

Just hit this a couple minutes ago 😄 thanks for opening a PR

Nuru
Nuru previously requested changes Jan 22, 2021
Copy link
Sponsor Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

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

I am not convinced this is really backwards compatible. The new Kubernetes provider drops support for KUBECONFIG, load_config_file, and the default ~/.kube/config as a default config_path. What will this break, how will clients fix it?

@woz5999
Copy link
Contributor Author

woz5999 commented Jan 22, 2021

@Nuru load_config_file was always kind of redundant with config_file. The new provider behavior does the logical thing and effectively determines whether or not to load the config file based on whether or not config_file is set. So previously, ~/.kube/config was ignored if load_config_file was false. This maintains that behavior.

For those that are loading a config file, chances are good they're already providing a path, and if they're not and relying on the default, the v2 provider would be a breaking change for them anyway. So for that segment of users, they'll have a v2 breaking change and just need to explicitly provide their path.

Leaving the default path in the variable will break this for everyone who previously had load_config_file set to false, which was the old variable default.

@mergify
Copy link

mergify bot commented Jan 22, 2021

This pull request is now in conflict. Could you fix it @woz5999? 🙏

@mergify mergify bot dismissed Nuru’s stale review January 22, 2021 01:39

This Pull Request has been updated, so we're dismissing all reviews.

@woz5999
Copy link
Contributor Author

woz5999 commented Jan 22, 2021

/test all

Nuru
Nuru previously requested changes Jan 22, 2021
Copy link
Sponsor Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

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

I think we can make this backwards compatible instead. Hang on please while I investigate.

@Nuru Nuru self-requested a review January 22, 2021 03:39
@mergify mergify bot dismissed Nuru’s stale review January 22, 2021 03:47

This Pull Request has been updated, so we're dismissing all reviews.

@Nuru
Copy link
Sponsor Contributor

Nuru commented Jan 22, 2021

/test all

@woz5999 after consulting with the team, we decide just to remove these variables altogether as they have no good use case for this module, and then move forward with the Kubernetes provider version pin >= 2.0. Thank you for your efforts and for your patience.

@Nuru Nuru merged commit bef1d41 into cloudposse:master Jan 22, 2021
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.

None yet

3 participants