-
Notifications
You must be signed in to change notification settings - Fork 8
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
Creation and destruction of Google recipes with 1 single command #151 #155
Conversation
10050bc
to
91da837
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for breaking this down to a more specific PR, I added some comments to get your feedback.
|
||
data "google_client_config" "default" {} | ||
|
||
resource "local_file" "kube_config_yaml" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm probably overlooking something basic, won't this be needed afterwards to install Rancher and interact with the cluster?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read further below, the kubeconfig templating is pushed to the recipe.. ideally we would remove as much logic from the recipe as possible and use it only as a high-level grouping of modules.
Eg, each module is to do one job and do it well, that way it can be used directly, even without the recipe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to have a reference in the recipe main to get the dependencies correct.
Otherwise, the provider initializes a value that does not exist when executing the deploy command and does not update it when it is actually created.
By doing so, the provider is initialized only when the file exists in the recipe path.
This code wasn't there when the deploy command was longer and I manually targeted the execution order.
default = "10.10.0.0/24" | ||
description = "Range of private IPs available for the Google Subnet" | ||
} | ||
# variable "ip_cidr_range" {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could copy these as-is from the module so they can be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I wrote all the repos, I left the minimum values necessary for deployment uncommented, while still leaving all the possible usable values commented out.
If you have a directive for this, I will implement it.
As an example:
- leave the minimal variables, but delete the unused ones;
- leave all the variables available with the default values configured both in the main and in the module;
- etc.
Let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think taking option 2 would be best within reason, keeping all options available for direct use, without having to update the variables.tf and uncomment or populate
One main benefit is this would make the docs.md of recipes more of a useful reference
This can be a separate issue/PR to review all recipes if you feel it's better to keep separate, but I think other recipes are already doing this, or pretty close
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs to refer to should be the modules; there are all the available fields with the available resources.
The recipe main contains only what needs to be modified; that's why everything is commented on.
Anyway, you tell me. This is a style choice and I adapt to what you tell me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with Derek. I'd rather the end user only update the terraform.tfvars and not have to uncomment code in the main.tf and variables.tf. That also makes it more difficult for users to pull in new changes.
cat <<EOF | kubectl apply -f - | ||
kind: ClusterRoleBinding | ||
apiVersion: rbac.authorization.k8s.io/v1 | ||
metadata: | ||
name: client-binding | ||
subjects: | ||
- kind: User | ||
name: client | ||
roleRef: | ||
kind: ClusterRole | ||
name: "cluster-admin" | ||
apiGroup: rbac.authorization.k8s.io | ||
EOF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be accomplished with the related terraform resource? https://registry.terraform.io/providers/hashicorp/kubernetes/latest/docs/resources/cluster_role_binding_v1
# Installation of the NGINX Ingress Controller (helm_release resource has destruction issues). | ||
kubectl create namespace ingress-nginx || true | ||
helm repo add ingress-nginx https://kubernetes.github.io/ingress-nginx | ||
helm install nginx ingress-nginx/ingress-nginx --namespace ingress-nginx --set controller.service.type="LoadBalancer" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the ingress install occurs here, any reason we can't do this with a helm_release
resource with wait
and wait_for_jobs
?
https://registry.terraform.io/providers/hashicorp/helm/latest/docs/resources/release#wait
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the apply and destroy. I had no issues. I did not closely review the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some unresolved comments but I want to progress this, functionally it's working, the comments may be nice to address in future.
#151