Skip to content

Conversation

@aaron-lane
Copy link
Contributor

This branch defines the root module. 🎉

@ocervell
Copy link

Looks good already. Any reason to not use https://github.com/terraform-google-modules/terraform-google-log-export for the log export ?

@aaron-lane
Copy link
Contributor Author

@ocervell I avoided terraform-google-log-export because I had hit terraform-google-modules/terraform-google-log-export#10 in the past, but on further inspection, it probably won't affect this module so I'll test it out.

@adrienthebo adrienthebo self-requested a review February 16, 2019 01:21
@aaron-lane aaron-lane force-pushed the aaron-lane/root-module-redux branch from f1f5320 to d81b86b Compare February 19, 2019 17:22
@aaron-lane
Copy link
Contributor Author

@ocervell Testing revealed that terraform-google-modules/terraform-google-log-export#10 does affect this module so using log-export is not an option at the present.

Copy link
Contributor

@adrienthebo adrienthebo left a comment

Choose a reason for hiding this comment

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

Review summary

Looks good, no blockers, I've a question or two and a few suggestions. Some questions regarding scoping might be too broad for this PR, so feel free to resolve those comments as you see fit.

Suggestions

  • Consider adding a random suffix to the GCS bucket
  • Consider dropping the name variable default value

Questions

  • What event types do we intend to support for the v1 release of this module?
  • What's the scope of the event function sources?

@aaron-lane aaron-lane force-pushed the aaron-lane/root-module-redux branch from e071d57 to 48baff7 Compare February 19, 2019 20:12
Copy link
Contributor

@adrienthebo adrienthebo left a comment

Choose a reason for hiding this comment

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

I'm happy with this, 👍 for merge. CI is not passing but not because of any issues in this PR.

Integration tests look good locally:

Verifying automatic_labelling

Profile: automatic_labelling
Version: 0.1.0
Target:  gcp://project-factory-20712@thebo-host-c15e.iam.gserviceaccount.com

  ✔  automatically labelled: Instance unlabelled
     ✔  Instance unlabelled should be automatically labelled by the Cloud Functions function

There's one whitespace error in the inspec.yml, with a local fix everything else looks good:

╰─ make check
Running shellcheck
Running flake8
Running go fmt and go vet
Running terraform validate
terraform validate --check-variables=false .
terraform validate --check-variables=false ./examples/automatic_labelling
terraform validate --check-variables=false ./test/fixtures/automatic_labelling
Running hadolint on Dockerfiles
Checking for required files LICENSE README.md
Testing the validity of the header check
..
----------------------------------------------------------------------
Ran 2 tests in 0.038s

OK
Checking file headers
Checking for trailing whitespace

@aaron-lane do you want to shave the CI yak here, or shall we handle that in a new PR?

@adrienthebo
Copy link
Contributor

Since this is functionally correct and I've verified it locally I'm going to merge it, and we can shave yaks later.

@adrienthebo adrienthebo merged commit 47ec5a2 into master Feb 19, 2019
@adrienthebo adrienthebo deleted the aaron-lane/root-module-redux branch February 19, 2019 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants