terraform-google-modules / terraform-google-startup-scripts

Provides a library of useful startup scripts to embed in VMs

https://registry.terraform.io/modules/terraform-google-modules/startup-scripts/google

Apache License 2.0

73 stars 36 forks source link

Add startup script library debugging functions #1

Closed jeffmccune closed 5 years ago

jeffmccune commented 5 years ago

This commit adds the basic structure of the startup script library. The intent is to focus on logging functions and hand off of control to the startup-script-custom script.

See examples/hello_world/ for usage information and the README for example output.

Testing

BATS unit tests are being ported over, this pull request is up for early comment.

adrienthebo commented 5 years ago

The addition of the project_id and region variables broke the example; I got things working with the following:

diff --git i/examples/simple_example/main.tf w/examples/simple_example/main.tf
index 10cd920..4afb5be 100644
--- i/examples/simple_example/main.tf
+++ w/examples/simple_example/main.tf
@@ -23,6 +23,9 @@ provider "google" {

 module "startup-scripts" {
   source = "../../"
+
+  project_id = "${var.project_id}"
+  region     = "${var.region}"
 }

 data "google_compute_image" "os" {

Do we need those variables defined on the module?

Once those changes applied the module worked as expected.

adrienthebo commented 5 years ago

Currently there are two outstanding issues:

:+1: following the resolution of those issues. With respect to BATS tests we can add them in this pull request, or we can follow up with another pull request (which I'd prefer over having a single monolithic PR.)

morgante commented 5 years ago

+1 to adding testing in a follow-up PR

jeffmccune commented 5 years ago

The example not working without project_id and region is unexpected. It should work without you needing to add them, and the module itself is intended to take no input variables so I’ll check that out first thing tomorrow morning and get it sorted.

Will also fix TMPDIR which should be straightforward. Plan to add stdlib::mktemp with the same behavior as now, only difference is TMPDIR won’t be exported.

Noted regarding tests, will add and follow up.

jeffmccune commented 5 years ago

Currently there are two outstanding issues:

#1 (comment)

#1 (comment)

👍 following the resolution of those issues. With respect to BATS tests we can add them in this pull request, or we can follow up with another pull request (which I'd prefer over having a single monolithic PR.)

Comments have been addressed in subsequent patches. I went ahead and rebased in ad57d83...ee304af to avoid an additional round trip to clean up the commits.

adrienthebo commented 5 years ago

The extra variables was an error on my part; I used the cookiecutter template to get access to the standard linters and got an extra variables.tf along the way.

jeffmccune commented 5 years ago

@adrienthebo FYI, spec test structure is up for review at https://github.com/terraform-google-modules/terraform-google-startup-scripts/pull/1