opendatacube / datacube-k8s-eks

Deploy a production scale datacube cluster on AWS using EKS
Apache License 2.0
21 stars 14 forks source link

Remove extra sw lib #230

Closed NikitaGandhi closed 4 years ago

NikitaGandhi commented 4 years ago

Any PRs will require running terraform fmt -recursive successfully first. Please install terraform version v0.12.18 on your local setup for this activity.

Why this change is needed

Describe why this change is needed, what issues it will fix and the benefits the change will add

This change refactor and removes install_extra_software.tf file from odc_k8s module to simplify CI tooling from infrastructure.

Negative effects of this change

Will making this change break or change an existing functionality? flag it here

Yes, it will impact @woodcockr. Suggestion is to put this lib to your internal module and pass kube_config_cmd if required otherwise kube_config_cmd is set to aws eks update-kubeconfig --name=${local.cluster_name} --region=${var.region}.

kube_config_cmd = "aws eks update-kubeconfig --name=${local.cluster_name} --region=${var.region} --kubeconfig=${var.kubeconfig_path} ${var.aws_eks_update_kubeconfig_additional_arguments}"
NikitaGandhi commented 4 years ago

Install extra software needs to be done as part of the terraform scripting at this point as the tooling cannot be preinstalled in Terraform Cloud workers. I don’t believe it can be performed through a separate module as it needs to be part of the same tf apply.

What is the problem with leaving it there given it’s optional? It’s a common pattern, used in other terraform modules like Cloud Posse.

Hey Rob, is it possible for you to install it from your internal odc_k8s live module (where you calling this module from). It is same thing and less messy in generic module??

woodcockr commented 4 years ago

As I mentioned is a common pattern in modules far more generic than this one.

I don’t think it will work from outside the module as its a dependency IN the module, not an outside one. Particularly in the case when a destroy is executed.

I also do not agree this is messy, it is in its own block of code isolated from everything else and doesn’t intrude at all for the case when it is disabled. It simply has a dependency that in the case where you happen to not be using TF Cloud or otherwise have locally installed tools it does nothing.

NikitaGandhi commented 4 years ago

ok. removing this PR.

woodcockr commented 4 years ago

The underlying issue is the helm operator crd install and the lack of terraform k8s provider support for CRDs at the time.

Would be worth checking if these have been patched (it was a raised issue at one point in the provider). If it is supported now then not only can the install extra software be removed but so can the aws eks command line.

woodcockr commented 4 years ago

Here we go, just made alpha release for k8s 1.17: https://www.hashicorp.com/blog/deploy-any-resource-with-the-new-kubernetes-provider-for-hashicorp-terraform/

I’m inclined to wait for final release - what we have works for the moment

NikitaGandhi commented 4 years ago

sure, let's raise an issue so we won't forget 👍