Skip to content

Conversation

@g-awmalik
Copy link
Contributor

No description provided.

@g-awmalik g-awmalik requested review from a team, donmccasland and glasnt as code owners May 20, 2023 06:05
Copy link
Contributor

@grayside grayside left a comment

Choose a reason for hiding this comment

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

Did some initial review but I'm still working to understand enough of the context.

Comment on lines +115 to +127
- level: Project
roles:
- roles/cloudsql.admin
- roles/compute.admin
- roles/compute.networkAdmin
- roles/firebase.managementServiceAgent
- roles/firebasehosting.admin
- roles/iam.serviceAccountAdmin
- roles/iam.serviceAccountUser
- roles/resourcemanager.projectIamAdmin
- roles/run.admin
- roles/secretmanager.admin
- roles/storage.admin
Copy link
Contributor

Choose a reason for hiding this comment

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

note: I compared these to https://github.com/GoogleCloudPlatform/terraform-dynamic-python-webapp/blob/main/infra/test/setup/iam.tf#L17 and they appear to be the same. Seems like we will need to keep them manually in sync, unless we can parse the values from this config into the iam.tf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

infra/test/setup/iam.tf is the source for this as you rightly point out and will be updated automatically. I don't follow your comment "we will need to keep them manually in sync". Is there's another config you're keeping these roles in?

description: A set of key/value label pairs to assign to the resources deployed by this blueprint.
varType: map(string)
defaultValue: {}
- name: 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.

question: Do these variables need to be manually synchronized with the terraform variables, or is something automatic going to happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are updated automatically with the TF variables defined for the blueprint.

Comment on lines +129 to +131
- cloudresourcemanager.googleapis.com
- storage-api.googleapis.com
- serviceusage.googleapis.com
Copy link
Contributor

Choose a reason for hiding this comment

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

question: I see only three services listed here, shouldn't this include all the services needed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe these are the services required to invoke terraform, which will then itself enable the other services. They're similar to the services avocano gets you to manually enable before running the script

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The source for this is the activate_apis variable to the project factory module under test/.

- name: image_version
description: Version of the Container Registry image to use
varType: string
defaultValue: v1.7.0
Copy link
Contributor

Choose a reason for hiding this comment

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

note for maintainers: we'll want to follow-up this change with a release-please config to update this value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the avocano version, not the self-version. Updating this is usually manual, are you saying release-please can help with this overhead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

release-please won't help with this. This should updated in the TF variable when a newer version of the blueprint is released and the metadata will be updated accordingly.

@donmccasland donmccasland merged commit 7364389 into main May 24, 2023
@donmccasland donmccasland deleted the feat/metadata-setup branch May 24, 2023 15:56
@g-awmalik
Copy link
Contributor Author

g-awmalik commented May 31, 2023

Did some initial review but I'm still working to understand enough of the context.

@grayside - I somehow missed your comments from a week ago. Please let me know if anything is still unclear.

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.

4 participants