-
Notifications
You must be signed in to change notification settings - Fork 819
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
Add Terraform GKE and Helm modules tests with Terratest #1483
Add Terraform GKE and Helm modules tests with Terratest #1483
Conversation
Build Succeeded 👏 Build Id: 303de1e1-8da6-474e-93e7-2e248626d179 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Saving go test file from this PR in IDE, changes root |
823f136
to
2d75daa
Compare
Build Succeeded 👏 Build Id: f198327a-df67-4e8d-be5a-87dee0184fba The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Well it seems reasonable to split out terraform config files into a separate repo. This would speed up using all |
74e7506
to
7b317e6
Compare
This Terratest output (head and tail):
|
Build Succeeded 👏 Build Id: 67e0ad44-ab32-4805-8a14-8ab1acd06768 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: e70e5ce0-c911-4af7-b94a-aae5114b3bd0 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
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.
This is really, really cool.
Do you envisage we should be doing this in our CI build pipeline?
(As per splitting out, maybe let's start a new ticket, work out a design of how we want to do that? Definitely not against it. Easy enough to configure a new Cloud Build setup, or maybe prow, also need to consider backward compatibility.)
test/terraform/gke_test.go
Outdated
func destroy(t *testing.T, options *terraform.Options) { | ||
options.Targets = []string{"module.helm_agones.helm_release.agones"} | ||
terraform.Destroy(t, options) | ||
time.Sleep(30 * time.Second) |
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.
Curious - is there a way we could poll to see if this is complete, rather than waiting for a period of time? Waiting is probably pretty fragile.
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.
Gentle bump on this review.
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.
Returning back to this as well
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.
Added the wait of kubectl services -n agones-system
count would be 0 using terratest K8S
module. Some updates to development .tf
file was needed - creating kubeconfig file.
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 is no other options to poll the result or at least nothing else come to my mind. But previous approach build a "bridge" for creating more complex tests.
diff --git a/go.mod b/go.mod
index 0e80fa99..268cd898 100644
--- a/go.mod
+++ b/go.mod
@@ -3,50 +3,42 @@ module agones.dev/agones
go 1.13
require (
- cloud.google.com/go v0.34.0
+ cloud.google.com/go v0.51.0
contrib.go.opencensus.io/exporter/prometheus v0.1.0
contrib.go.opencensus.io/exporter/stackdriver v0.8.0
fortio.org/fortio v1.3.1
+ git.apache.org/thrift.git v0.0.0-20180902110319-2566ecd5d999 // indirect
github.com/ahmetb/gen-crd-api-reference-docs v0.1.1
- github.com/aws/aws-sdk-go v1.16.20 // indirect
github.com/evanphx/json-patch v4.5.0+incompatible // indirect
github.com/fsnotify/fsnotify v1.4.7
- github.com/go-openapi/spec v0.19.0
- github.com/gogo/protobuf v1.2.1 // indirect
+ github.com/go-openapi/spec v0.19.3 Seems to be |
Trying the following solution to avoid updating root |
7b317e6
to
b6dd0e5
Compare
Build Failed 😱 Build Id: e6ab66b7-591e-40dc-ae8d-2843676f3c82 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
d9cb72e
to
fba1b05
Compare
Build Failed 😱 Build Id: 1b828e64-96be-4fd0-972a-1164ce6fa673 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
This PR is ready to review now. |
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.
Sorry, missed this on last review
build/includes/terraform.mk
Outdated
@@ -84,3 +86,18 @@ gcloud-terraform-destroy-cluster: GCP_PROJECT ?= $(current_project) | |||
gcloud-terraform-destroy-cluster: | |||
$(DOCKER_RUN) bash -c 'cd $(mount_path)/build/terraform/gke && terraform destroy -var project=$(GCP_PROJECT) -auto-approve' | |||
|
|||
terraform-test: $(ensure-build-image) | |||
ifndef GCP_PROJECT | |||
$(eval GCP_PROJECT=$(shell sh -c "gcloud config get-value project 2> /dev/null")) |
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.
This shouldn't be necessary if we put current_project
back in.
@@ -15,17 +15,19 @@ | |||
# The GKE development cluster name | |||
GCP_TF_CLUSTER_NAME ?= agones-tf-cluster | |||
|
|||
# the current project | |||
current_project := $(shell $(DOCKER_RUN) bash -c "gcloud config get-value project 2> /dev/null") |
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.
Just realised we're deleting current_project
- this will break stuff 😢
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.
Oops, sorry for that, returning it back.
Applying PR comments. Add gitignore.
Update terraform.Destroy first target pass.
Running test with -project equals empty string is forbiden and nonsense.
f62841f
to
34bcd8c
Compare
This comment has been minimized.
This comment has been minimized.
This would get the necessary project name from docker run.
34bcd8c
to
960800b
Compare
Build Succeeded 👏 Build Id: e5dc3a1d-4dde-41d9-912f-0309a5dae3bf The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 53305c71-862c-4ed3-811d-cde99c077dcc The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Fixed all comments, this PR is good to go now. |
Build Succeeded 👏 Build Id: 70b0f1f0-5776-4095-aed8-98c738f1be08 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aLekSer, markmandel The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
New changes are detected. LGTM label has been removed. |
Build Succeeded 👏 Build Id: 33fe73e2-d74e-4149-bd59-a30ced240005 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
…es#1483) * Enhance tests, use kubectl module to verify svc Co-authored-by: Mark Mandel <markmandel@google.com>
Useful to check if Terraform configuration is valid.
What type of PR is this?
/kind feature
What this PR does / Why we need it:
Which issue(s) this PR fixes:
Closes #1227.
Special notes for your reviewer:
There is a need to make local run of the test in a way that root
go.mod
andgo.sum
does not get affected.This Test framework also good at verification of different variables used by helm templates
https://github.com/gruntwork-io/terratest/blob/master/test/helm_basic_example_template_test.go and https://github.com/gruntwork-io/terratest-helm-testing-example .