Allow project name to be changed separately from project ID #154

thefirstofthe300 · 2019-03-01T19:42:53Z

Fixes #132.

aaron-lane

The project_id interface needs to be added to the gsuite_enabled module.

We should test these changes against the minimal suite converged from the master branch. Are there any unexpected consequences just from updating? Can the name be changed without destroying resources?

thefirstofthe300 · 2019-03-05T02:48:42Z

The project_id interface needs to be added to the gsuite_enabled module.

I think you are referring to this change?

We should test these changes against the minimal suite converged from the master branch. Are there any unexpected consequences just from updating? Can the name be changed without destroying resources?

https://gist.github.com/thefirstofthe300/d72f2d563196e1fce1ab36b734e64fdb

The minimal test converges properly. The full test suite recreates the project because the project ID changes. The precondition being rebuilt is due to an old container version being used and causing the shell command to fail. I'm going to be submitting a PR tomorrow to update the container and make targets since the targets themselves fail right now.

aaron-lane · 2019-03-05T16:20:22Z

@thefirstofthe300:

Apologies: I missed the changes to gsuite_enabled. 😊

With respect to testing the full suite, can you verify the impact of changing only the name?

morgante

Mostly looks good except dropping local.project_id.

morgante · 2019-03-06T01:18:29Z

modules/core_project_factory/main.tf

  s_account_fmt          = "${format("serviceAccount:%s", google_service_account.default_service_account.email)}"
-  api_s_account          = "${format("%[email protected]", local.project_number)}"
+  api_s_account          = "${format("%[email protected]", google_project.main.number)}"


Why can't this continue to reference project_number?

The net effect of this change is a no-op. It removes a level of indirection since local.project_number is referring to google_project.main.number. I see no purpose for continuing to use local.project_number. It makes things a bit more difficult to read so I would much prefer to remove it.

morgante · 2019-03-06T01:18:50Z

modules/core_project_factory/main.tf

@@ -26,18 +26,17 @@ resource "random_id" "random_project_id_suffix" {
 *****************************************/
 locals {
  group_id               = "${var.manage_group ? format("group:%s", var.group_email) : ""}"
-  project_id             = "${google_project.main.project_id}"
-  project_number         = "${google_project.main.number}"


Why drop local.project_number?

The local here is a level of indirection that is unnecessary, IMHO.

It's easier to manage this in one place instead of having to update it on every resource.

morgante · 2019-03-06T01:19:59Z

modules/core_project_factory/main.tf

  project_org_id         = "${var.folder_id != "" ? "" : var.org_id}"
  project_folder_id      = "${var.folder_id != "" ? var.folder_id : ""}"
-  temp_project_id        = "${var.random_project_id ? format("%s-%s",var.name,random_id.random_project_id_suffix.hex) : var.name}"
+  project_id             = "${var.project_id == "" ? var.name : var.project_id}"


I think we should continue to have project_id reference the actual project created.

We can move the conditional check into the project ID itself.

This local doesn't necessarily refer to the value that will be used to set the project's ID. I'll rename these variables to make the naming a bit more sane.

morgante · 2019-03-06T01:21:06Z

modules/core_project_factory/main.tf

@@ -104,7 +103,7 @@ resource "google_resource_manager_lien" "lien" {
 resource "google_project_service" "project_services" {
  count = "${length(var.activate_apis)}"

-  project = "${local.project_id}"
+  project = "${google_project.main.project_id}"


We should keep local.project_id unless there's a reason we can't, similarly for all cases of this replacement.

I am of the opinion that the project_id local should be removed or repurposed to make the project_id generation more clear. It also adds a layer of redirection that seems unnecessary so my comments on dropping the project_number local applies as well.

thefirstofthe300

Made a couple of tweaks around local naming.

I'm not sold on the idea of using locals that are equivalent to resource outputs. We should be using the resource outputs directly and avoid declaring those locals.

thefirstofthe300 · 2019-03-06T18:32:25Z

modules/core_project_factory/main.tf

@@ -26,18 +26,17 @@ resource "random_id" "random_project_id_suffix" {
 *****************************************/
 locals {
  group_id               = "${var.manage_group ? format("group:%s", var.group_email) : ""}"
-  project_id             = "${google_project.main.project_id}"
-  project_number         = "${google_project.main.number}"


The local here is a level of indirection that is unnecessary, IMHO.

thefirstofthe300 · 2019-03-06T18:35:04Z

modules/core_project_factory/main.tf

  s_account_fmt          = "${format("serviceAccount:%s", google_service_account.default_service_account.email)}"
-  api_s_account          = "${format("%[email protected]", local.project_number)}"
+  api_s_account          = "${format("%[email protected]", google_project.main.number)}"


The net effect of this change is a no-op. It removes a level of indirection since local.project_number is referring to google_project.main.number. I see no purpose for continuing to use local.project_number. It makes things a bit more difficult to read so I would much prefer to remove it.

thefirstofthe300 · 2019-03-06T18:39:35Z

modules/core_project_factory/main.tf

@@ -104,7 +103,7 @@ resource "google_resource_manager_lien" "lien" {
 resource "google_project_service" "project_services" {
  count = "${length(var.activate_apis)}"

-  project = "${local.project_id}"
+  project = "${google_project.main.project_id}"


I am of the opinion that the project_id local should be removed or repurposed to make the project_id generation more clear. It also adds a layer of redirection that seems unnecessary so my comments on dropping the project_number local applies as well.

morgante

We can merge, but I'm unconvinced that spreading ${google_project.main.project_id} into a dozen locations instead of referencing ${local.project_id} everywhere is superior.

morgante · 2019-03-06T21:46:29Z

test/integration/full/controls/project-factory.rb

@@ -51,10 +63,6 @@
      end
    end

-    it "includes the Google App Engine API service account user" do


@thefirstofthe300 Please do not introduce completely unrelated changes into a different PR. Is this somehow related to the other functionality in this PR and I missed it?

thefirstofthe300 marked this pull request as ready for review March 1, 2019 19:58

thefirstofthe300 requested review from aaron-lane, adrienthebo and morgante as code owners March 1, 2019 19:58

thefirstofthe300 force-pushed the ds/import-projects branch from 02542e6 to e0d5c4c Compare March 5, 2019 00:07

aaron-lane suggested changes Mar 5, 2019

View reviewed changes

thefirstofthe300 force-pushed the ds/import-projects branch from e0d5c4c to 039dd58 Compare March 6, 2019 01:01

morgante suggested changes Mar 6, 2019

View reviewed changes

thefirstofthe300 force-pushed the ds/import-projects branch from 039dd58 to 1fbfbdd Compare March 6, 2019 18:41

thefirstofthe300 commented Mar 6, 2019

View reviewed changes

thefirstofthe300 force-pushed the ds/import-projects branch from 1fbfbdd to a2f818d Compare March 6, 2019 18:52

Allow project name to be changed separately from project ID

1dbdaba

thefirstofthe300 force-pushed the ds/import-projects branch from a2f818d to 6b3182c Compare March 6, 2019 19:03

morgante previously approved these changes Mar 6, 2019

View reviewed changes

Add tests for checking project ID and project name

07a4a56

thefirstofthe300 dismissed morgante’s stale review via 07a4a56 March 6, 2019 20:33

thefirstofthe300 force-pushed the ds/import-projects branch from 6b3182c to 07a4a56 Compare March 6, 2019 20:33

morgante suggested changes Mar 6, 2019

View reviewed changes

thefirstofthe300 force-pushed the ds/import-projects branch from 8326d7e to 07a4a56 Compare March 6, 2019 22:26

thefirstofthe300 mentioned this pull request Mar 6, 2019

Remove test for app engine service account creation #162

Merged

morgante approved these changes Mar 6, 2019

View reviewed changes

morgante merged commit 30770cf into terraform-google-modules:master Mar 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow project name to be changed separately from project ID #154

Allow project name to be changed separately from project ID #154

thefirstofthe300 commented Mar 1, 2019

aaron-lane left a comment

thefirstofthe300 commented Mar 5, 2019

aaron-lane commented Mar 5, 2019

morgante left a comment

morgante Mar 6, 2019

thefirstofthe300 Mar 6, 2019

morgante Mar 6, 2019

thefirstofthe300 Mar 6, 2019

morgante Mar 6, 2019 •

edited

Loading

morgante Mar 6, 2019

thefirstofthe300 Mar 6, 2019

morgante Mar 6, 2019

thefirstofthe300 Mar 6, 2019

thefirstofthe300 left a comment

thefirstofthe300 Mar 6, 2019

thefirstofthe300 Mar 6, 2019

thefirstofthe300 Mar 6, 2019

morgante left a comment

morgante Mar 6, 2019

Allow project name to be changed separately from project ID #154

Allow project name to be changed separately from project ID #154

Conversation

thefirstofthe300 commented Mar 1, 2019

aaron-lane left a comment

Choose a reason for hiding this comment

thefirstofthe300 commented Mar 5, 2019

aaron-lane commented Mar 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morgante Mar 6, 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

thefirstofthe300 left a 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

morgante left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morgante Mar 6, 2019 •

edited

Loading