remove k8s_version variable

[ettiee]

We provided a variable for k8s_version in the module. This would allow users to set a different k8s version to the github release they are using, which may cause them to use incorrect configuration for that k8s version and use the wrong version of some addons.

We should make it clear that this isn't configurable and instead users should use the appropriate github release

[takanabe]

We should make it clear that this isn't configurable and instead users should use the appropriate github release

Could you elaborate on the reason why you think so?I think the version is configurable and we don't need this PR.

Let's say we want to deploy EKS cluster using K8s version 1.17 and 1.19 we can use the variable to specify the versions.

• https://docs.aws.amazon.com/eks/latest/userguide/retrieve-ami-id.html
• https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/eks_cluster#version

[ettiee]

Could you elaborate on the reason why you think so?I think the version is configurable and we don't need this PR.

Sure! Let's say a user wants to use the module to set up a cluster with k8s 1.19. They should use the terraform cluster module targeting version 1.19. Let's say that user now wants to install a cluster with k8s 1.17, and they use that same terraform module (version 1.19) but set k8s_version = 1.17.0. This will configure the control plane and node template to version 1.17, but some other configuration in the module might cause bugs for k8s 1.17.0. For example the addons will be using the versions recommended for k8s 1.19 and might cause bugs on k8s 1.17. There might be some configuration dropped in a later version that's required in an earlier version of k8s - this might be missed if someone uses a later version of the terraform module and configures k8_version to be for an earlier release.

So two reasons for me to open this PR:

• It's confusing for users to configure the k8s version in two places (as a variable, and by pulling the appropriate version of the terraform module)
• Might cause bugs if users set a different k8s_version to the one that the release was tested against

[takanabe]

Thank you for the explanation. Now, your intention is clear and I think this changes make sense. So, will we change all supported versions' k8s_version variables to local scopes?

[ettiee]

Thank you for the explanation. Now, your intention is clear and I think this changes make sense. So, will we change all supported versions' k8s_version variables to local scopes?

Yes - do you agree with the implementation (change all supported versions' k8s_version variables to local scopes)?

[takanabe]

Yes. When is the timing we declare this is a breaking change for the module? Maybe at the time to release a new patch version for each minor version?

[ettiee]

Yes we should announce the breaking change with the release. What releases do you think we should target this PR to? I think we should release this with the next minor version 1.20, but I'm not sure we need to release a patch to previous minor version.

📝 I will add a note to https://github.com/cookpad/terraform-aws-eks/blob/main/UPGRADING.md about the breaking change

[errm]

LGTM

[takanabe]

LGTM

[mtpereira]

❤️