Storage Notification Resource #1033

ishashchuk · 2018-01-31T19:44:56Z

Support for GSC Notifications issue 971

API reference here

The resource closely mimics the API functionality. if no event_types are passed, ALL events will be publishing to pubsub topic. If event_types are passed, only specific events will be published

There is no update functionality; updating the notification will always result in new notification config

Storage Default Object ACL resource

Cleaning up storage_default_object_acl

Storage notifications

emilymye

Thanks for adding this!

emilymye · 2018-02-01T19:02:32Z

google/resource_storage_notification.go

+				},
+			},
+
+			"prefix": &schema.Schema{


We tend to prefer having this match the resource exactly. Would it be possible to rename this as "object_name_prefix"?

emilymye · 2018-02-01T19:08:38Z

google/resource_storage_notification.go

+	}
+
+	res, err := config.clientStorage.Notifications.Insert(bucket, storageNotification).Do()
+


nit - can you remove the newline here? this is more personal preference but generally we don't have an empty line between _, err := ... and if err != nil {} blocks

emilymye · 2018-02-01T19:08:48Z

google/resource_storage_notification.go

+	notificationID := resourceStorageNotificationParseID(d.Id())
+
+	res, err := config.clientStorage.Notifications.Get(bucket, notificationID).Do()
+


nit - newline

emilymye · 2018-02-01T19:24:54Z

google/resource_storage_notification.go

+				Type:     schema.TypeString,
+				Required: true,
+				ForceNew: true,
+			},


Can you add a DiffSuppressFunc here? Example in PubSub topic resource

emilymye · 2018-02-01T19:29:47Z

google/resource_storage_notification.go

+		EventTypes:       convertStringSet(d.Get("event_types").(*schema.Set)),
+		ObjectNamePrefix: d.Get("prefix").(string),
+		PayloadFormat:    d.Get("payload_format").(string),
+		Topic:            d.Get("topic").(string),


It might be best to do some regex check here and reformat if needed. It's likely someone would pass in a topic self-link or relative name ("projects/%s/topics/%s) here.

emilymye · 2018-02-01T19:35:35Z

google/resource_storage_notification.go

+	}
+
+	d.Set("payload_format", res.PayloadFormat)
+	d.Set("topic", strings.SplitAfter(res.Topic, "//pubsub.googleapis.com/")[1])


Not sure if this will happen anytime soon, but if the topic doesn't have this value anymore, SplitAfter will return only one elem here and this will panic. You can also use the getRelativePath self-link helper func.

emilymye · 2018-02-01T19:38:03Z

google/resource_storage_notification.go

+	notificationID := resourceStorageNotificationParseID(d.Id())
+
+	err := config.clientStorage.Notifications.Delete(bucket, notificationID).Do()
+


nit - newline

emilymye · 2018-02-01T20:56:44Z

google/resource_storage_notification.go

+}
+
+func resourceStorageNotificationImportState(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
+	parts := strings.Split(d.Id(), "/")


It might be nicer if we just have this in resourceStorageNotificationParseID, and have resourceStorageNotificationParseID return both bucket and notificationConfig id.

emilymye · 2018-02-01T21:00:00Z

website/docs/r/storage_notification.html.markdown

+
+- - -
+
+* `custom_attrubutes` - (Optional)  A set of key/value attribute pairs to attach to each Cloud PubSub message published for this notification subscription


s/custom_attrubutes/custom_attributes

Addressing requested changes from review

danawillow · 2018-02-02T23:10:25Z

google/resource_storage_notification.go

+func resourceStorageNotificationRead(d *schema.ResourceData, meta interface{}) error {
+	config := meta.(*Config)
+
+	bucket := d.Get("bucket").(string)


Since the bucket is already part of the id, you could avoid this d.Get here by using the return value from the parse function, which would then allow you to use an ImportStatePassthrough

danawillow · 2018-02-02T23:14:54Z

google/resource_storage_notification.go

+		return handleNotFoundError(err, d, fmt.Sprintf("Notification configuration %s for bucket %s", notificationID, bucket))
+	}
+
+	topic, _ := getRelativePath(res.Topic)


Since we have the DiffSuppressFunc for self link or resource name, we should be fine just storing this the way it's returned from the API (either way is really fine, but since that's the case may as well go with the simpler solution)

danawillow · 2018-02-02T23:17:40Z

google/resource_storage_notification_test.go

+
+func testGoogleStorageNotificationBasic(bucketName, topicName, topic string) string {
+	return fmt.Sprintf(`
+		resource "google_storage_bucket" "bucket" {


nit: we try to have these all the way on the left (mostly a personal preference thing, it just makes it a bit easier to copy/paste into our own configs later, feel free to ignore)

danawillow · 2018-02-02T23:17:57Z

google/resource_storage_notification_test.go

+		// Otherwise notification configuration won't work
+		resource "google_pubsub_topic_iam_binding" "binding" {
+			topic = "${google_pubsub_topic.topic.name}"
+			role        = "roles/pubsub.publisher"


nit: align =s (here and elsewhere)

danawillow · 2018-02-02T23:19:02Z

google/resource_storage_notification_test.go

+)
+
+var (
+	gscServiceAccount = fmt.Sprintf("serviceAccount:%[email protected]", os.Getenv("GOOGLE_PROJECT"))


should this be gcs? or does gsc stand for something?

danawillow · 2018-02-02T23:20:14Z

google/resource_storage_notification_test.go

+
+		bucket := rs.Primary.Attributes["bucket"]
+
+		_, notificationID := resourceStorageNotificationParseID(rs.Primary.ID)


Since the bucket is in the id also, you could just do bucket, notificationID here

danawillow · 2018-02-02T23:20:36Z

google/resource_storage_notification_test.go

+
+func testAccCheckStorageNotificationCheckEventType(notification *storage.Notification, eventTypes []string) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+


nit: extra newline

danawillow · 2018-02-02T23:22:37Z

website/docs/r/storage_notification.html.markdown

+	name = "default_topic"
+}
+
+//In order to enable notifications,


nit: Can you add a space after each //?

danawillow · 2018-02-02T23:23:53Z

website/docs/r/storage_notification.html.markdown

+```
+$ terraform import google_storage_notification.notification default_bucket/notificationConfigs/102
+```
+


nit: there are a few extra lines here that can be deleted

danawillow · 2018-02-02T23:27:44Z

website/docs/r/storage_notification.html.markdown

+
+## Import
+
+Storage notifications can be imported using the notification `id` in the format `<bucket_name>/notificationConfigs/<id>` e.g.


I wonder whether it makes more sense to just have it be <bucket_name>/<id> that way people don't have to bother typing out "/notificationConfigs/" (unless there's somewhere where that string tends to be copy/pasted from). What do you think?

Also, do you think most people will generally know the id of the notification? If there can only be one notification per topic per bucket then you could use the topic name as something more easily identifiable. If not though then it probably has to be the id.

I was debating on the way to id them too.. The reason I decided to go with the <bucket_name>/notificationConfigs/ is because that's how they are being returned in CLI util:

gsutil notifications list gs://test_bucket/ projects/_/buckets/test_bucket/notificationConfigs/89 Cloud Pub/Sub topic: projects/sandbox/topics/test_topic

It seems to me that this would be one of the quickest and user friendly ways to get the list of notifications on the bucket, in case they need to be imported and the id can be just copy/pasted. What do you think?

Yup, that's a great reason. Thanks for the explanation!

Addressing requested changes from review

danawillow · 2018-02-05T18:53:48Z

google/resource_storage_notification.go

+	return nil
+}
+
+func resourceStorageNotificationImportState(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {


This function can be removed now (you can just set the importer to schema.ImportStatePassthrough, see other resources for examples), and then you can do the d.Set call inside the read fn

danawillow · 2018-02-05T18:56:39Z

website/docs/r/storage_notification.html.markdown

+
+## Import
+
+Storage notifications can be imported using the notification `id` in the format `<bucket_name>/notificationConfigs/<id>` e.g.


Yup, that's a great reason. Thanks for the explanation!

emilymye · 2018-02-05T21:03:30Z

I'm going to approve, but I'd wait for Dana's approval before actually merging.

Using ImportStatePassthrough

danawillow

Thanks @ishashchuk, looks good!

(I'll go ahead and merge this @emilymye since I'm already here)

* Storage Default Object ACL resource * Fixed the doc * Renamed the resource id. Log change * Complying with go vet * Changes for review * link to default object acl docs in sidebar * Support for GCS notifications * docs for storage notification * docs for storage notification * Clarified the doc * Doc modifications * Addressing requested changes from review * Addressing requested changes from review * Using ImportStatePassthrough

Signed-off-by: Modular Magician <[email protected]>

ghost · 2020-03-29T14:31:40Z

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 👉 [email protected]. Thanks!

ishashchuk and others added 18 commits January 18, 2018 14:43

Storage Default Object ACL resource

ba3c60c

Fixed the doc

19912ec

Renamed the resource id. Log change

6810e53

Merged latest upstream changes into the branch

2c80aee

Merge pull request #1 from wayfair/storage_default_acl

58c7e0e

Storage Default Object ACL resource

Complying with go vet

e7cebf7

Changes for review

5e35381

Merge remote-tracking branch 'upstream/master'

c702a73

Merge pull request #2 from wayfair/ishashchuk_storage_defaul_acl

bca2b1b

Cleaning up storage_default_object_acl

link to default object acl docs in sidebar

a89bc5d

Support for GCS notifications

61d2db7

Merge remote-tracking branch 'upstream/master'

bff7fd5

Merge branch 'master' into ishashchuk_storage_notifications

5c1e5b9

docs for storage notification

14ebfc9

docs for storage notification

7f5951c

Clarified the doc

79a38d9

Doc modifications

d224374

Merge pull request #3 from wayfair/ishashchuk_storage_notifications

d7ec385

Storage notifications

emilymye requested a review from danawillow February 1, 2018 18:28

emilymye suggested changes Feb 1, 2018

View reviewed changes

ishashchuk and others added 2 commits February 2, 2018 13:05

Addressing requested changes from review

f153c0f

Merge pull request #4 from wayfair/ishashchuk_storage_notifications

e5a0d27

Addressing requested changes from review

danawillow reviewed Feb 2, 2018

View reviewed changes

ishashchuk and others added 2 commits February 4, 2018 20:52

Addressing requested changes from review

0d00aff

Merge pull request #6 from wayfair/ishashchuk_notifications

93dd896

Addressing requested changes from review

danawillow reviewed Feb 5, 2018

View reviewed changes

emilymye approved these changes Feb 5, 2018

View reviewed changes

ishashchuk and others added 2 commits February 5, 2018 16:39

Using ImportStatePassthrough

4794d18

Merge pull request #7 from wayfair/ishashchuk_notifications

c4b6a26

Using ImportStatePassthrough

danawillow approved these changes Feb 5, 2018

View reviewed changes

danawillow merged commit 7257b9a into hashicorp:master Feb 5, 2018

danawillow mentioned this pull request Feb 5, 2018

GCS: Notifications [feature request] #971

Closed

rosbo mentioned this pull request Feb 16, 2018

Feature request: support for storage object change notifications via pubsub #761

Closed

modular-magician added a commit to modular-magician/terraform-provider-google that referenced this pull request Sep 27, 2019

Add Cloud SCC Source resource (hashicorp#1033)

5a6917d

Signed-off-by: Modular Magician <[email protected]>

ghost locked and limited conversation to collaborators Mar 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage Notification Resource #1033

Storage Notification Resource #1033

ishashchuk commented Jan 31, 2018

emilymye left a comment

emilymye Feb 1, 2018

emilymye Feb 1, 2018

emilymye Feb 1, 2018

emilymye Feb 1, 2018

emilymye Feb 1, 2018

emilymye Feb 1, 2018

emilymye Feb 1, 2018

emilymye Feb 1, 2018

emilymye Feb 1, 2018

danawillow Feb 2, 2018

danawillow Feb 2, 2018

danawillow Feb 2, 2018

danawillow Feb 2, 2018

danawillow Feb 2, 2018

danawillow Feb 2, 2018

danawillow Feb 2, 2018

danawillow Feb 2, 2018

danawillow Feb 2, 2018

danawillow Feb 2, 2018

ishashchuk Feb 3, 2018

danawillow Feb 5, 2018

danawillow Feb 5, 2018

danawillow Feb 5, 2018

emilymye commented Feb 5, 2018

danawillow left a comment

ghost commented Mar 29, 2020

		}

		res, err := config.clientStorage.Notifications.Insert(bucket, storageNotification).Do()

		notificationID := resourceStorageNotificationParseID(d.Id())

		res, err := config.clientStorage.Notifications.Get(bucket, notificationID).Do()

		notificationID := resourceStorageNotificationParseID(d.Id())

		err := config.clientStorage.Notifications.Delete(bucket, notificationID).Do()


		- - -

		* `custom_attrubutes` - (Optional) A set of key/value attribute pairs to attach to each Cloud PubSub message published for this notification subscription


		bucket := rs.Primary.Attributes["bucket"]

		_, notificationID := resourceStorageNotificationParseID(rs.Primary.ID)


		func testAccCheckStorageNotificationCheckEventType(notification *storage.Notification, eventTypes []string) resource.TestCheckFunc {
		return func(s *terraform.State) error {


		## Import

		Storage notifications can be imported using the notification `id` in the format `<bucket_name>/notificationConfigs/<id>` e.g.

Storage Notification Resource #1033

Storage Notification Resource #1033

Conversation

ishashchuk commented Jan 31, 2018

emilymye left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilymye commented Feb 5, 2018

danawillow left a comment

Choose a reason for hiding this comment

ghost commented Mar 29, 2020