Adding new App Engine support #4
Conversation
|
@ryanckoch Can you please add a test to check that an app is actually created on the project? |
|
@morgante Done |
| app_engine_enabled = "${length(keys(var.app_engine)) > 0 ? true : false}" | ||
|
|
||
| app_engine_config = { | ||
| enabled = "${list(var.app_engine)}" |
There was a problem hiding this comment.
Does this actually work? var.app_engine is a map, not a list.
There was a problem hiding this comment.
It gets the length of keys in app_engine which is a list
There was a problem hiding this comment.
But then I'm confused about how line 86 works? https://github.com/terraform-google-modules/terraform-google-project-factory/pull/4/files/a51eb1f65e51143af5b7dff92d9ce2fa268cfb38#diff-7a370d8342e7203b805911c92454f0f4R86
Where do we actually pass in the keys from the map?
There was a problem hiding this comment.
local.app_engine_config is a map of lists with keys enabled and disabled. Line 86 is referencing that list by key depending if local.app_engine_enabled is true or false. If local.app_engine_enabled is true, it sets app_engine to the enabled list which is the users config, otherwise it uses the disabled list which is empty. It is a dirty hack, but the only way I could see to pass a completely empty list to app_engine and still be able to accept the app_engine variable as a map in our module. Otherwise, it would set app_engine = [{}] which isnt an empty list and tells the provider to setill enable app_engine.
main.tf
Outdated
| @@ -119,6 +127,23 @@ resource "null_resource" "delete_default_compute_service_account" { | |||
| depends_on = ["google_project_service.project_services"] | |||
| } | |||
|
|
|||
| /****************************************** | |||
| Default app engine service account deletion | |||
There was a problem hiding this comment.
@ryanckoch We should not delete the app engine service account—deleting it will break App Engine.
| @@ -76,6 +82,8 @@ resource "google_project" "project" { | |||
| auto_create_network = "${var.auto_create_network}" | |||
|
|
|||
| labels = "${var.labels}" | |||
|
|
|||
| app_engine = "${local.app_engine_config["${local.app_engine_enabled ? "enabled" : "disabled"}"]}" | |||
There was a problem hiding this comment.
@ryanckoch This is hard to parse and somewhat confusing.
Wouldn't app_engine = "${local.app_engine_enabled ? var.app_engine : list()}" work just as well?
There was a problem hiding this comment.
It wont allow for a list in a conditional. That would definitely be much cleaner, but that is the reason for the hack app_engine_config map.
There was a problem hiding this comment.
How about "${local.app_engine_enabled ? local.app_engine_config["enabled"] : local.app_engine_config["disabled"]}"
There was a problem hiding this comment.
Same:
Error: module.project-factory.google_project.project: 1 error(s) occurred:
* module.project-factory.google_project.project: At column 3, line 1: conditional operator cannot be used with list values in:
${local.app_engine_enabled ? local.app_engine_config["enabled"] : local.app_engine_config["disabled"]}
| @@ -76,6 +82,8 @@ resource "google_project" "project" { | |||
| auto_create_network = "${var.auto_create_network}" | |||
|
|
|||
| labels = "${var.labels}" | |||
|
|
|||
| app_engine = "${local.app_engine_config["${local.app_engine_enabled ? "enabled" : "disabled"}"]}" | |||
There was a problem hiding this comment.
Also worth noting, that although the examples in the documentation show app_engine as accepting a map, it accepts a list with a max length of 1.
https://github.com/terraform-providers/terraform-provider-google/blob/master/google/resource_google_project.go#L95
Have created an issue on the provider: hashicorp/terraform-provider-google#1821
|
Please address this comment and improve the app engine integration test by using (a) a different region + (b) an additional non-default app_engine flag + verifying both are set corectly. |
|
@morgante The above comment has been addressed. It is worth noting that setting custom feature_settings requires multiple applies: hashicorp/terraform-provider-google#1826 |
merging with project factory master
…odules/aaron-lane-v0.1.0 Add CHANGELOG entry for 0.1.0
See hashicorp/terraform-provider-google#1503 for context.
Merge after: #3