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 simple bucket submodule #35

Merged
merged 12 commits into from
Feb 13, 2020
Merged

Add simple bucket submodule #35

merged 12 commits into from
Feb 13, 2020

Conversation

umairidris
Copy link
Contributor

@umairidris umairidris commented Feb 11, 2020

This module provides a simpler module for creating a single bucket. This is more consistent with the Terraform provider and other CFT modules.

It provides better defaults out of the box as well than using the provider directly:

  • versioning set to true
  • bucket policy only set to true

@umairidris umairidris requested a review from morgante February 11, 2020 18:10
Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

Looks like the permissions on examples/multi_buckets were changed. Can you please fix and ensure tests are also not broken?

name = "example-bucket"
project = "example-project"
location = "us-east1"
iam_members = [{
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of requiring users to set IAM roles themselves (and potentially misconfiguring IAM), could we use bucket_admins, bucket_creators, and bucket_viewers variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be clear, you mean helper iam fields in addition to iam_members?

If we require a helper field for every role it can get quite large and somewhat inflexible I imagine (esp if using custom roles, etc like in #18).

Also other modules like healthcare, etc also just have a single iam_members field.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair point, I was mostly thinking about consistency with the existing module. I guess it's a nice-to-have but not required.

Agreed with should always support explicit iam_members.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great. I'll hold off on this for the initial version. I think it's worth it for users to understand the exact roles they are assigning as there are some subtle differences e.g. roles/storage.admin vs roles/storage.objectAdmin. We can definitely expand it in the future as you mentioned alongside explicit iam_members.

type = string
}

variable "project" {
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, let's call this project_id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

@umairidris umairidris left a comment

Choose a reason for hiding this comment

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

Fixed tests as well

name = "example-bucket"
project = "example-project"
location = "us-east1"
iam_members = [{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great. I'll hold off on this for the initial version. I think it's worth it for users to understand the exact roles they are assigning as there are some subtle differences e.g. roles/storage.admin vs roles/storage.objectAdmin. We can definitely expand it in the future as you mentioned alongside explicit iam_members.

type = string
}

variable "project" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

Just some doc nits.


The resources/services/activations/deletions that this module will create/trigger are:

- One GCS buckets
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
- One GCS buckets
- One GCS buckets

The resources/services/activations/deletions that this module will create/trigger are:

- One GCS buckets
- Zero or more IAM bindings for those buckets
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
- Zero or more IAM bindings for those buckets
- Zero or more IAM bindings for that bucket

## Compatibility

This module is meant for use with Terraform 0.12. If you haven't [upgraded](https://www.terraform.io/upgrade-guides/0-12.html)
and need a Terraform 0.11.x-compatible version of this module, the last released version intended for
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no version of this module for TF 0.11. Could probably drop this whole section.

The following dependencies must be available:

- [Terraform][terraform] v0.12
- For Terraform v0.11 see the [Compatibility](#compatibility) section above
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove.

@umairidris
Copy link
Contributor Author

D'oh. Rushed through the docs earlier. Fixed

@umairidris umairidris requested a review from morgante February 12, 2020 02:25
Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

Two small nits/typos.

@umairidris umairidris requested a review from morgante February 12, 2020 15:00
@morgante morgante merged commit e75114a into terraform-google-modules:master Feb 13, 2020
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