Update testing to new approach #33

ivankorn · 2019-09-23T15:37:17Z

Fixes #29

Migrated to Cloud Build as documented here.

Please note following deviations from migration document:

exact list of roles on the project (and other resources) has been granted instead of owner
for_each instead of count.index is used to create a number of instances of resource.
pause of 120 sec (determined experimentally) is used to wait for APIs to get enabled as a workaround for the provider issue
pause of 120 sec (determined experimentally) is used to wait for permissions to get granted as a workaround for the provider issue

test/setup/main.tf

ingwarr · 2019-09-23T15:51:59Z

test/setup/main.tf

+  version = "~> 3.0"
+
+  name              = "ci-{{ cookiecutter.module_name|replace('-', '_') }}"
+  random_project_id = "true"


It boolean variable, so quotes should be removed

@ingwarr, thank you for review! Fixed

Except you didn't fix it...

Good eye, @morgante! Fixed
But actually, shouldn't we raise this in the module that came from ?
Thanks!

ivankorn · 2019-09-27T14:55:12Z

CI is green, RP got 2 approvals. @morgante, @aaron-lane, would you mind reviewing it please?

Please note following deviations from migration document:

exact list of roles on the project (and other resources) has been granted instead of owner
for_each instead of count.index is used to create a number of instances of resource.
pause of 120 sec (determined experimentally) is used to wait for APIs to get enabled as a workaround for the provider issue
pause of 120 sec (determined experimentally) is used to wait for permissions to get granted as a workaround for the provider issue

morgante

Overall looking good, some minor feedback.

test/setup/make_source.sh

test/setup/main.tf

morgante · 2019-09-27T15:04:13Z

test/setup/main.tf

+  version = "~> 3.0"
+
+  name              = "ci-{{ cookiecutter.module_name|replace('-', '_') }}"
+  random_project_id = "true"


Except you didn't fix it...

CONTRIBUTING.md

morgante · 2019-09-27T15:10:00Z

test/setup/make_source.sh

+
+# shellcheck disable=SC2086,SC2154
+{ echo "export TF_VAR_project_id='$project_id'"; \
+echo "export TF_VAR_parent_resource_project='$project_id'"; \


We shouldn't need to output this twice. Just update the tests to take project_id as the input variable name.

@morgante, should we, what if in some cases it's different ?

It should never be different.

morgante · 2019-09-27T15:10:41Z

test/setup/iam.tf

+  ]
+
+  log_export_billing_account_roles = [
+    # Required to associate billing accounts to new projects


Are you sure we need this? The tests themselves shouldn't be creating any new projects (the project creation happens in the prepare step).

@morgante, the prepare creates project while the computed values tests is doing association to billing account which requires the billing user and the project creator roles for the tests them-self.

morgante · 2019-09-27T15:10:43Z

test/setup/iam.tf

+    "roles/logging.configWriter",
+
+    # Required to associate billing accounts to new projects
+    "roles/billing.projectManager",


Why needed? We don't create new projects in fixtures.

The prepare creates project while the computed values tests is doing association to billing account

morgante · 2019-09-27T15:11:04Z

test/setup/iam.tf

+
+  log_export_folder_roles = [
+    # Required to spin up a project within the log_export folder
+    "roles/resourcemanager.projectCreator",


Again, where are we creating a new project besides in the setup phase?

Yes, this is meant for the setup phase so the required project for running tests can be created.

Right, so that's not required. Please try to understand how this actually executes instead of putting in unnecessary permissions.

test/setup is applied using the Cloud Build service account. So permissions needed only to run the setup stage do NOT need to be added here.

@morgante, sorry for confusion. I just double-checked that with this force-push. The permission is required for tests them-self for the computed values test suite and can't take it out.
It's used here

ivankorn

@morgante thank you for in-depth review! Fixed and commented all the comments!

ivankorn · 2019-09-27T16:13:18Z

test/setup/iam.tf

+
+  log_export_folder_roles = [
+    # Required to spin up a project within the log_export folder
+    "roles/resourcemanager.projectCreator",


Yes, this is meant for the setup phase so the required project for running tests can be created.

ivankorn · 2019-09-27T16:22:45Z

test/setup/make_source.sh

+
+# shellcheck disable=SC2086,SC2154
+{ echo "export TF_VAR_project_id='$project_id'"; \
+echo "export TF_VAR_parent_resource_project='$project_id'"; \


@morgante, should we, what if in some cases it's different ?

ivankorn · 2019-09-27T16:31:15Z

test/setup/iam.tf

+    "roles/logging.configWriter",
+
+    # Required to associate billing accounts to new projects
+    "roles/billing.projectManager",


The prepare creates project while the computed values tests is doing association to billing account

ivankorn · 2019-09-27T16:31:28Z

test/setup/iam.tf

+  ]
+
+  log_export_billing_account_roles = [
+    # Required to associate billing accounts to new projects


@morgante, the prepare creates project while the computed values tests is doing association to billing account which requires the billing user and the project creator roles for the tests them-self.

test/setup/main.tf

test/setup/make_source.sh

ivankorn · 2019-09-27T16:42:39Z

test/setup/main.tf

+  version = "~> 3.0"
+
+  name              = "ci-{{ cookiecutter.module_name|replace('-', '_') }}"
+  random_project_id = "true"


Good eye, @morgante! Fixed
But actually, shouldn't we raise this in the module that came from ?
Thanks!

CONTRIBUTING.md

morgante · 2019-10-08T21:41:25Z

test/setup/make_source.sh

+
+# shellcheck disable=SC2086,SC2154
+{ echo "export TF_VAR_project_id='$project_id'"; \
+echo "export TF_VAR_parent_resource_project='$project_id'"; \


It should never be different.

ingwarr suggested changes Sep 23, 2019

View reviewed changes

ivankorn force-pushed the 29 branch from 07b614e to 2976460 Compare September 25, 2019 13:19

ivankorn requested review from ingwarr, kopachevsky, nick4fake and paulpalamarchuk September 25, 2019 13:35

aaron-lane added the enhancement New feature or request label Sep 25, 2019

ivankorn force-pushed the 29 branch 3 times, most recently from 30b2256 to 03d8031 Compare September 25, 2019 16:11

kopachevsky approved these changes Sep 26, 2019

View reviewed changes

ivankorn force-pushed the 29 branch 6 times, most recently from 0a6ff54 to 59dfa95 Compare September 26, 2019 17:07

ingwarr approved these changes Sep 26, 2019

View reviewed changes

ivankorn force-pushed the 29 branch 7 times, most recently from 3650d09 to d9bd46a Compare September 27, 2019 14:28

ivankorn marked this pull request as ready for review September 27, 2019 14:55

morgante suggested changes Sep 27, 2019

View reviewed changes

ivankorn force-pushed the 29 branch from d9bd46a to 21c7079 Compare September 27, 2019 16:41

ivankorn commented Sep 27, 2019

View reviewed changes

ivankorn force-pushed the 29 branch from 21c7079 to 0a1a212 Compare September 30, 2019 14:37

Migrated to Cloud Build terraform-google-modules#29

3b701bf

ivankorn force-pushed the 29 branch from 0a1a212 to 3b701bf Compare September 30, 2019 16:01

ivankorn requested review from morgante and kopachevsky September 30, 2019 16:05

morgante self-assigned this Oct 7, 2019

morgante approved these changes Oct 8, 2019

View reviewed changes

morgante merged commit 56394ac into terraform-google-modules:master Oct 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update testing to new approach #33

Update testing to new approach #33

ivankorn commented Sep 23, 2019 •

edited

Loading

ingwarr Sep 23, 2019

ivankorn Sep 25, 2019

morgante Sep 27, 2019

ivankorn Sep 27, 2019

ivankorn commented Sep 27, 2019

morgante left a comment

morgante Sep 27, 2019

morgante Sep 27, 2019

ivankorn Sep 27, 2019 •

edited

Loading

morgante Oct 8, 2019

morgante Sep 27, 2019

ivankorn Sep 27, 2019 •

edited

Loading

morgante Sep 27, 2019

ivankorn Sep 27, 2019

morgante Sep 27, 2019

ivankorn Sep 27, 2019

morgante Sep 27, 2019

ivankorn Sep 30, 2019

ivankorn left a comment

ivankorn Sep 27, 2019

ivankorn Sep 27, 2019 •

edited

Loading

ivankorn Sep 27, 2019

ivankorn Sep 27, 2019 •

edited

Loading

ivankorn Sep 27, 2019

morgante Oct 8, 2019

Update testing to new approach #33

Update testing to new approach #33

Conversation

ivankorn commented Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivankorn commented Sep 27, 2019

morgante left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivankorn Sep 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivankorn Sep 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivankorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivankorn Sep 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivankorn Sep 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivankorn commented Sep 23, 2019 •

edited

Loading

ivankorn Sep 27, 2019 •

edited

Loading

ivankorn Sep 27, 2019 •

edited

Loading

ivankorn Sep 27, 2019 •

edited

Loading

ivankorn Sep 27, 2019 •

edited

Loading