-
Notifications
You must be signed in to change notification settings - Fork 171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: migrating to TheLook Ecommerce dataset #257
feat!: migrating to TheLook Ecommerce dataset #257
Conversation
Initial migration to thelook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good start! a few things around dependencies and what the linter will dislike to resolve.
modules/data_warehouse/bigquery.tf
Outdated
|
||
labels = var.labels | ||
|
||
depends_on = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove these since the solution calls out the resources specifically above (applies to other tables below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaned up these dependencies, as well as those across all .tf files
modules/data_warehouse/bigquery.tf
Outdated
table_id = "events" | ||
project = module.project-services.project_id | ||
deletion_protection = var.deletion_protection | ||
# max_staleness = "1:0:0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we arent planning to use these, I'd probably just remove them to keep the code simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept them in here because I thought it would be good for folks to see the syntax, but commented them out so that the Looker Studio report works immediately.
modules/data_warehouse/bigquery.tf
Outdated
@@ -182,6 +305,8 @@ resource "google_project_iam_member" "dts_roles" { | |||
project = module.project-services.project_id | |||
role = each.key | |||
member = "serviceAccount:${google_service_account.dts.email}" | |||
|
|||
depends_on = [google_service_account.dts] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one isnt needed since you have an output attribute of DTS resource on 307
) | ||
GROUP BY user_id | ||
) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add new lines
modules/data_warehouse/variables.tf
Outdated
@@ -39,13 +39,13 @@ variable "enable_apis" { | |||
variable "force_destroy" { | |||
type = string | |||
description = "Whether or not to protect BigQuery resources from deletion when solution is modified or changed." | |||
default = false | |||
default = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we set these to the non-destructive option by default in the code (we override them in Solution Deployment)
modules/data_warehouse/variables.tf
Outdated
} | ||
|
||
variable "deletion_protection" { | ||
type = string | ||
description = "Whether or not to protect GCS resources from deletion when solution is modified or changed." | ||
default = true | ||
default = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above.
modules/data_warehouse/workflows.tf
Outdated
project = module.project-services.project_id | ||
role = each.key | ||
member = "serviceAccount:${google_service_account.workflow_service_account.email}" | ||
|
||
|
||
depends_on = [google_service_account.workflow_service_account] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed.
Gotcha. I'd probably remove them to just keep the code simpler and we can
look at if we need to add an example in the terraform resource provider.
…On Thu, Oct 5, 2023 at 8:58 AM Shane Glass ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In modules/data_warehouse/bigquery.tf
<#257 (comment)>
:
> @@ -48,22 +51,127 @@ resource "google_storage_bucket_iam_binding" "bq_connection_iam_object_viewer" {
]
}
-# # Create a BigQuery external table
-resource "google_bigquery_table" "tbl_edw_taxi" {
+# # Create a Biglake table for events with metadata caching
+resource "google_bigquery_table" "tbl_edw_events" {
+ dataset_id = google_bigquery_dataset.ds_edw.dataset_id
+ table_id = "events"
+ project = module.project-services.project_id
+ deletion_protection = var.deletion_protection
+ # max_staleness = "1:0:0"
I kept them in here because I thought it would be good for folks to see
the syntax, but commented them out so that the Looker Studio report works
immediately.
—
Reply to this email directly, view it on GitHub
<#257 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADK7DPSPA3PZTM4ZSJ3NQWLX53DHXAVCNFSM6AAAAAA5PSUEBOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMNRQGA4TINZTGI>
.
You are receiving this because your review was requested.Message ID:
<terraform-google-modules/terraform-google-bigquery/pull/257/review/1660094732
@github.com>
|
/gcbrun |
/gcbrun |
1 similar comment
/gcbrun |
…dules-master' into terraform-google-modules-master
/gcbrun |
/gcbrun |
1 similar comment
/gcbrun |
commit f88f4b5 Author: Shane Glass <[email protected]> Date: Mon Oct 23 15:10:22 2023 -0400 feat: data_warehouse Add GenAI capabilities (terraform-google-modules#272) commit b7efc4d Author: Awais Malik <[email protected]> Date: Thu Oct 19 15:38:43 2023 -0700 fix: adds a null check for expiration time (terraform-google-modules#268) commit 405972a Author: Shane Glass <[email protected]> Date: Tue Oct 17 17:03:10 2023 -0400 fix: update workflow.tftpl (terraform-google-modules#266) commit 1046e0d Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Tue Oct 17 09:29:48 2023 -0700 chore(deps): update cft/developer-tools docker tag to v1.17 (terraform-google-modules#265) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit ceef798 Author: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Date: Tue Oct 17 09:48:20 2023 -0600 chore(master): release 7.0.0 (terraform-google-modules#249) commit 1a7620b Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Tue Oct 10 09:11:32 2023 -0700 chore(deps): Update module github.com/GoogleCloudPlatform/cloud-foundation-toolkit/infra/blueprint-test to v0.9.0 (terraform-google-modules#260) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit 7fd5bcb Author: Awais Malik <[email protected]> Date: Mon Oct 9 14:33:52 2023 -0700 fix: upgraded versions.tf to include minor bumps from tpg v5 (terraform-google-modules#261) Co-authored-by: Andrew Peabody <[email protected]> Co-authored-by: Jason Davenport <[email protected]> commit e97adfb Author: Shane Glass <[email protected]> Date: Mon Oct 9 15:27:46 2023 -0400 feat!: data_warehosue migrating to TheLook Ecommerce dataset (terraform-google-modules#257) commit 096ca4e Author: Lloyd Armstrong <[email protected]> Date: Mon Oct 9 18:28:35 2023 +0200 fix: upgrade hashicorp/google to 5.0 (terraform-google-modules#259) Co-authored-by: Lloyd Armstrong <[email protected]>
Squashed commit of the following: commit f88f4b5 Author: Shane Glass <[email protected]> Date: Mon Oct 23 15:10:22 2023 -0400 feat: data_warehouse Add GenAI capabilities (terraform-google-modules#272) commit b7efc4d Author: Awais Malik <[email protected]> Date: Thu Oct 19 15:38:43 2023 -0700 fix: adds a null check for expiration time (terraform-google-modules#268) commit 405972a Author: Shane Glass <[email protected]> Date: Tue Oct 17 17:03:10 2023 -0400 fix: update workflow.tftpl (terraform-google-modules#266) commit 1046e0d Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Tue Oct 17 09:29:48 2023 -0700 chore(deps): update cft/developer-tools docker tag to v1.17 (terraform-google-modules#265) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit ceef798 Author: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Date: Tue Oct 17 09:48:20 2023 -0600 chore(master): release 7.0.0 (terraform-google-modules#249) commit 1a7620b Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Tue Oct 10 09:11:32 2023 -0700 chore(deps): Update module github.com/GoogleCloudPlatform/cloud-foundation-toolkit/infra/blueprint-test to v0.9.0 (terraform-google-modules#260) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit 7fd5bcb Author: Awais Malik <[email protected]> Date: Mon Oct 9 14:33:52 2023 -0700 fix: upgraded versions.tf to include minor bumps from tpg v5 (terraform-google-modules#261) Co-authored-by: Andrew Peabody <[email protected]> Co-authored-by: Jason Davenport <[email protected]> commit e97adfb Author: Shane Glass <[email protected]> Date: Mon Oct 9 15:27:46 2023 -0400 feat!: data_warehosue migrating to TheLook Ecommerce dataset (terraform-google-modules#257) commit 096ca4e Author: Lloyd Armstrong <[email protected]> Date: Mon Oct 9 18:28:35 2023 +0200 fix: upgrade hashicorp/google to 5.0 (terraform-google-modules#259) Co-authored-by: Lloyd Armstrong <[email protected]>
No description provided.