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 IAM support for pubsub topic #875

Merged
merged 5 commits into from
Dec 20, 2017
Merged

Conversation

rosbo
Copy link
Contributor

@rosbo rosbo commented Dec 19, 2017

Fixes #83

cc @ndmckinley (Can't assign you until you re-accept the contributor invite).

@rosbo
Copy link
Contributor Author

rosbo commented Dec 19, 2017

--- PASS: TestAccPubsubTopicIamPolicy (4.25s)
--- PASS: TestAccPubsubTopicIamMember (5.49s)
--- PASS: TestAccPubsubTopicIamBinding (7.41s)

@rosbo rosbo removed the request for review from danawillow December 19, 2017 01:42
}

func (u *PubsubTopicIamUpdater) GetMutexKey() string {
return fmt.Sprintf("iam-folder-%s", u.topic)
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be updated

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 keep forgetting this one... Done

}

func (u *PubsubTopicIamUpdater) DescribeResource() string {
return fmt.Sprintf("folder %q", u.topic)
Copy link
Contributor

Choose a reason for hiding this comment

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

and this

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

}

// v1 and v2beta policy are identical
func resourceManagerToPubsubPolicy(in *cloudresourcemanager.Policy) (*pubsub.Policy, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this func follows a different format than the one in iam_storage_bucket.go, consider using a consistent style between the two

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They use the same format:

  • resourceManagerToStoragePolicy resourceManagerToPubsubPolicy
  • storageToResourceManagerPolicy vs pubsubToResourceManagerPolicy

Copy link
Contributor

Choose a reason for hiding this comment

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

ah sorry I meant in using named return variables:

func resourceManagerToStoragePolicy(p *cloudresourcemanager.Policy) (policy *storage.Policy, err error) {
	policy = &storage.Policy{}
	err = Convert(p, policy)
	return
}

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. I kept the new way of doing it and changed iam storage bucket. The error message is more descriptive.

Copy link
Contributor

Choose a reason for hiding this comment

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

You'll also want to do it for iam_service_account.go and iam_kms_key_ring.go

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

Providers: testAccProviders,
Steps: []resource.TestStep{
{
// Test Iam Member creation (no update for member, no need to test)
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is misplaced.
Also, should we have an update test for this?

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

@rosbo rosbo merged commit 42cc44e into hashicorp:master Dec 20, 2017
@sachams
Copy link

sachams commented Dec 20, 2017

Just been looking for a way to permission pubsub topics! Do you have an idea when this feature might be released?

@rosbo rosbo deleted the iam-pubsub-topic branch December 21, 2017 00:11
chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
* Add IAM support for pubsub topic

* Fix resource name

* Add update test for iam_policy resource

* Standardize policy conversion function

* Standardize policy conversion function all resources
@ghost
Copy link

ghost commented Mar 30, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add IAM support for pubsub topic
3 participants