Skip to content

feat(google_permissions): Allow roles/datastore.user for folder, nonprod, and prod roles#449

Merged
jbuck merged 1 commit intomainfrom
jbuck/push-ouqpwpzunsyu
Mar 30, 2026
Merged

feat(google_permissions): Allow roles/datastore.user for folder, nonprod, and prod roles#449
jbuck merged 1 commit intomainfrom
jbuck/push-ouqpwpzunsyu

Conversation

@jbuck
Copy link
Copy Markdown
Member

@jbuck jbuck commented Mar 20, 2026

Description

This PR allows roles/datastore.user for folder, nonprod, and prod roles. I decided on a slightly different implementation scheme that should be a bit easier for CEs to manage roles. I could also move the existing resource blocks to use this instead, if that's desired.

Related Tickets & Documents

@github-actions github-actions bot added the minor This PR will increment a minor version label Mar 20, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Release plan

Directory Previous version New version
google_permissions 0.5.0 0.6.0

@jbuck jbuck force-pushed the jbuck/push-ouqpwpzunsyu branch 3 times, most recently from 2ba3f28 to cfe3ca0 Compare March 21, 2026 20:06
Copy link
Copy Markdown
Member

@whd whd left a comment

Choose a reason for hiding this comment

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

I could also move the existing resource blocks to use this instead, if that's desired.

I think that will be overall less confusing but will produce minor confusion in the short term i.e. you will probably want to manually roll this out everywhere if you do this and while you probably should, I can see the argument for letting dependabot handle the easy upgrade and save proper refactoring for when we've done upstream work to facilitate that like https://mozilla-hub.atlassian.net/browse/MZCLD-2140

@jbuck jbuck force-pushed the jbuck/push-ouqpwpzunsyu branch from cfe3ca0 to 294f002 Compare March 30, 2026 17:32
whd
whd previously approved these changes Mar 30, 2026
Copy link
Copy Markdown
Member

@whd whd left a comment

Choose a reason for hiding this comment

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

r+wc

I strongly recommend updating https://github.com/mozilla/terraform-modules/blob/main/google_permissions/ADDING_NEW_ROLE.md if this is the new preferred way to add roles.

precondition {
condition = alltrue([
for x in var.nonprod_roles : contains(local.project_additional_roles, x)
for x in var.nonprod_roles : contains(local.project_additional_roles, x) || contains(local.allowed_prod_roles, x)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
for x in var.nonprod_roles : contains(local.project_additional_roles, x) || contains(local.allowed_prod_roles, x)
for x in var.nonprod_roles : contains(local.project_additional_roles, x) || contains(local.allowed_nonprod_roles, x)

*/

locals {
allowed_folder_roles = [
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If there's a high probability we keep these lists synced it makes sense to deduplicate here. But that gets into the general structure of this module which is not a topic we want to get into.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, I think as part of the refactoring of existing roles into using local.allowed_folder_roles, local.allowed_nonprod_roles, and local.allowed_prod_roles that would be a good time to remove this list

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Er whoops, misread the context. I kept the original design where roles could be allowed at separate levels

precondition {
condition = alltrue([
for x in var.nonprod_roles : contains(local.project_additional_roles, x)
for x in var.nonprod_roles : contains(local.project_additional_roles, x) || contains(local.allowed_prod_roles, x)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
for x in var.nonprod_roles : contains(local.project_additional_roles, x) || contains(local.allowed_prod_roles, x)
for x in var.nonprod_roles : contains(local.project_additional_roles, x) || contains(local.allowed_nonprod_roles, x)

Copy link
Copy Markdown
Member

@whd whd left a comment

Choose a reason for hiding this comment

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

r+ the docs simplification though I have a bit of a hard time following the lines preliminary to the changes.

@jbuck jbuck merged commit 3c04667 into main Mar 30, 2026
10 checks passed
@jbuck jbuck deleted the jbuck/push-ouqpwpzunsyu branch March 30, 2026 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor This PR will increment a minor version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants