Skip to content
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

add config for gcp ga4 project #1484

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

add config for gcp ga4 project #1484

wants to merge 9 commits into from

Conversation

Nyzl
Copy link
Member

@Nyzl Nyzl commented Oct 28, 2024

Adds Terraform configuration of the GA4 project in GCP.
This covers project creation, service accounts, roles and project level permissions

@theseanything
Copy link
Contributor

@Nyzl if this is ready for review - could you assign reviewers? If not, please make it a draft.

@Nyzl Nyzl requested a review from a team November 11, 2024 11:59
terraform/deployments/ga4-analytics/main.tf Show resolved Hide resolved
terraform/deployments/ga4-analytics/project_iam_binding.tf Outdated Show resolved Hide resolved
]
}

resource "google_project_iam_binding" "project-GDS_BQ_read_access" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
resource "google_project_iam_binding" "project-GDS_BQ_read_access" {
resource "google_project_iam_binding" "project-gds-bq-read-access" {

Can we at least normalise the names for the terraform resources?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, much cleaner and easier to read

Copy link
Contributor

@theseanything theseanything Nov 25, 2024

Choose a reason for hiding this comment

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

Nit: normalise dashes "-" to underscores "_" as per terraform conventions. https://developer.hashicorp.com/terraform/language/style#resource-naming

terraform/deployments/ga4-analytics/service_account.tf Outdated Show resolved Hide resolved
terraform/deployments/ga4-analytics/service_account.tf Outdated Show resolved Hide resolved
terraform/deployments/ga4-analytics/project_iam_binding.tf Outdated Show resolved Hide resolved
@Nyzl Nyzl requested a review from theseanything November 13, 2024 10:18
create_ignore_already_exists = "true"
}

resource "google_service_account" "sa--search-analytics-pipeline" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
resource "google_service_account" "sa--search-analytics-pipeline" {
resource "google_service_account" "search_analytics_pipeline" {

Don't think the "sa--" prefix adds anything to the resources names. Also use "_" rather than "-"s

title = "GDS BQ user"
}

resource "google_project_iam_custom_role" "roles--gds_bigquery_editor" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
resource "google_project_iam_custom_role" "roles--gds_bigquery_editor" {
resource "google_project_iam_custom_role" "gds_bigquery_editor" {

Nit: don't think the prefix adds anything to the name

@@ -0,0 +1,144 @@
resource "google_project_iam_custom_role" "roles--gds_bigquery_read_access" {
description = "Created on: 2023-10-27"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a description which describes the purpose of the role?

@@ -0,0 +1,3 @@
output "google_project_id" {
value = google_project.project.project_id
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this being used by anything?

@@ -0,0 +1,25 @@
resource "google_service_account" "sa--ga4-analytics-352613" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
resource "google_service_account" "sa--ga4-analytics-352613" {
resource "google_service_account" "ga4-analytics" {

Avoiding naming with arbitrary descriptors (we don't need to know the project id in the tf resource name)

@@ -0,0 +1,25 @@
resource "google_service_account" "sa--ga4-analytics-352613" {
account_id = "ga4-analytics-352613"
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is meant to be the same as the project id - could we use a reference to the project id?

@Nyzl
Copy link
Member Author

Nyzl commented Nov 25, 2024

thank you @theseanything, it's good to be picky and get into the habit of writing to standard

@Nyzl Nyzl requested a review from theseanything November 25, 2024 16:55
@theseanything
Copy link
Contributor

theseanything commented Dec 3, 2024

@Nyzl any update on this PR? Looks ready to go - just 1 comment here that optional: https://github.com/alphagov/govuk-infrastructure/pull/1484/files#r1856384597

@Nyzl
Copy link
Member Author

Nyzl commented Dec 4, 2024

sorry @theseanything i always seem to get side tracked. Yes i think that final suggestion is good to go. Would you be able to merge please?

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.

2 participants