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

fix!: pass iam_user_emails as map to postgres module #414

Conversation

mnahkies
Copy link
Contributor

this solves the Error: Invalid for_each argument issue that occurs if you try to create a service account and database in the same root module.

(it's a bit awkward but to my knowledge this is the correct way to do it in terraform)

fixes issue #413

Notes
Obviously this is a breaking change - I expect we'll want to add migration instructions to https://github.com/mnahkies/terraform-google-sql-db/blob/7d6b2095f0e2312424c97a416dbb929d57c73eaa/docs/upgrading_to_sql_db_14.0.0.md or the v15 equivalent - ideally I'd like some feedback on whether this change will be accepted before producing these though.

I think it would be nice to get this into the v14 release, given that it already contains other IAM related breaking changes to the postgres module.

@mnahkies mnahkies requested a review from a team as a code owner January 21, 2023 11:11
@mnahkies mnahkies force-pushed the mn/413/iam-user-creation-failure branch from 76e90b2 to f95a008 Compare January 21, 2023 11:12
@mnahkies mnahkies changed the title fix!(postgres): pass iam_user_emails as map fix!: pass iam_user_emails as map to postgres module Jan 21, 2023
@comment-bot-dev
Copy link

@mnahkies
Thanks for the PR! 🚀
✅ Lint checks have passed.

@g-awmalik
Copy link
Contributor

Thanks for catching this @mnahkies. Can you make iam_user_emails an object instead of a map so it's clear on what we're setting as values? Something like this should work:

type = list(object({
    id  = string
    email = string
  }))

mnahkies and others added 3 commits March 12, 2023 08:06
this solves the "Error: Invalid for_each argument" issue that occurs if
you try to create a service account and database in the same root
module.

(it's a bit awkward but to my knowledge this is the correct way to do it
in `terraform`)

fixes issue terraform-google-modules#413
@mnahkies mnahkies force-pushed the mn/413/iam-user-creation-failure branch from 3ab6184 to 5f582c9 Compare March 12, 2023 08:28
modules/mssql/README.md Outdated Show resolved Hide resolved

```

We recommend using `moved` blocks as [documented here](https://developer.hashicorp.com/terraform/language/modules/develop/refactoring)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that the minimum supported terraform version is now 1.3.0 since v14 and so I think we should encourage the use of moved blocks over running terraform state mv commands, but let me know if you disagree

@mnahkies
Copy link
Contributor Author

Thanks for catching this @mnahkies. Can you make iam_user_emails an object instead of a map so it's clear on what we're setting as values? Something like this should work:

type = list(object({
    id  = string
    email = string
  }))

Apologies that it's taken me so long to get back to this @g-awmalik - I was bogged down with life stuff for a few weeks.

I've now:

  • Changed type to list(object(...)) as requested
  • Written upgrade/migration instructions
  • Synced with base branch

If you could please take another look that would be great :)

Co-authored-by: Kareem Daggash <kareemdaggash@gmail.com>
@mnahkies
Copy link
Contributor Author

@g-awmalik any chance you would have time to give this a re-review / approve a CI run?

@apeabody
Copy link
Contributor

/gcbrun

@g-awmalik
Copy link
Contributor

/gcbrun

@g-awmalik
Copy link
Contributor

/gcbrun

@g-awmalik g-awmalik merged commit 15298c2 into terraform-google-modules:master Apr 18, 2023
4 checks passed
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.

None yet

5 participants