Add support for multiple Pub/Sub notification configurations#2135
Add support for multiple Pub/Sub notification configurations#2135lberrymage wants to merge 3 commits into
Conversation
fake-gcs-server's flags currently support setting only a single object storage notification configuration, rendering it impossible to model certain system architectures which depend on multiple notification configurations (e.g., a system which sends notifications to different Pub/Sub topics depending on the bucket) even if these architectures are possible with Google Cloud Storage proper. This commit adds support for specifying a full notification configuration with the -event.config flag using semicolon-separated key-value pairs. The flag can be repeated to add additional notification configurations. Backward compatibility is preserved with the old -event.* flags (which can specify only a single notification config) by allowing them to work previously, but ignoring them if the new -event.config flag is used. The implementation uses a new MultiEventManager type which simply wraps a list of PubsubEventManagers. This approach allows for maximum code reuse with minimum code churn, although it might technically be cleaner to altogether replace the implementation of PubsubEventManager with that of MultiEventManager in the future.
fsouza
left a comment
fsouza
left a comment
There was a problem hiding this comment.
Thank you very much for contributing. Left a couple of comments, LGTM otherwise.
| // | ||
| // bucket, project, and topic are required. events defaults to finalize if not | ||
| // specified. prefix is optional. | ||
| func parseEventConfig(raw string) (notification.EventManagerOptions, error) { |
There was a problem hiding this comment.
not super concerning given how fake-gcs-server is used, but bucket=;project=;topic= passes validation because the has* booleans get set regardless of value content. NewPubsubEventManager then sees empty ProjectID/TopicName and silently creates a no-op manager. Can you update the code to validate that those values are not empty?
| fs.StringVar(&cfg.event.bucket, "event.bucket", "", "if not empty, only objects in this bucket will generate trigger events") | ||
| fs.StringVar(&cfg.event.prefix, "event.object-prefix", "", "if not empty, only objects having this prefix will generate trigger events") | ||
| fs.StringVar(&eventList, "event.list", eventFinalize, "comma separated list of events to publish on cloud function URl. Options are: finalize, delete, and metadataUpdate") | ||
| fs.Var(&cfg.events, "event.config", "notification configuration in the format: bucket=<bucket>;project=<project>;topic=<topic>[;events=<event1>,<event2>][;prefix=<prefix>]. Can be specified multiple times for multiple configurations. events defaults to finalize. Supported events: finalize, delete, metadataUpdate, archive") |
There was a problem hiding this comment.
can you add a warning or something like that when the user provides both flags? (like -event.pubsub-topic and -event.config)
There was a problem hiding this comment.
I can, though I think that might require more invasive changes (which I don't necessarily mind making) because there currently isn't a good way to differentiate between -event.list=finalize being passed explicitly and event.list defaulting to finalize when it isn't set. Imo, even though I didn't initially implement it this way, I think it might actually be better to return an error in this case since doing so would more aggressively prevent that mistake and since the error wouldn't appear for people already using only the old flags. An error could also be implemented in a simpler way anyway.
Would returning an error (e.g., in config validation) in this case instead of logging a warning and using the new flag's values be acceptable to you? Or would you prefer I add a warning to the existing approach?
This is both a correction and slight readability change to 9313f54: "Add support for multiple Pub/Sub notification configurations". First, it is a correction in that it fixes a few tests introduced in that commit to make them test the correct properties of the event config. For example, the "event.config unknown key" test should test for unknown keys specified in the event config; however, it can fail because the project key is not specified instead, making the test appear to test a property it doesn't. Second, this is a readability change in that it changes the values of test config properties to 1) match the ones existing before 9313f54 and 2) remove unnecessary optional values. As a result, it is much easier to see at a glance whether the config test data is correct for a given test.
|
I added another commit (5c5f7f8) to resolve some issues with the tests in the original commit and otherwise make them more readable. I left it as a separate commit for ease of review, but I can squash it if you'd prefer. |
|
That's an interesting add, I was looking into implementing the same thing, however, I was thinking more of setting this dynamically upon bucket creation. What do you say? @lberrymage I'd be happy to implement it on top of your codebase. |
3 participants
fake-gcs-server's flags currently support setting only a single object storage notification configuration, rendering it impossible to model certain system architectures which depend on multiple notification configurations (e.g., a system which sends notifications to different Pub/Sub topics depending on the bucket) even if these architectures are possible with Google Cloud Storage proper.
This commit adds support for specifying a full notification configuration with the -event.config flag using semicolon-separated key-value pairs. The flag can be repeated to add additional notification configurations. Backward compatibility is preserved with the old event.* flags (which can specify only a single notification config) by allowing them to work previously, but ignoring them if the new -event.config flag is used. The list of event types which trigger a notification defaults to "finalize" to match existing behavior with the old -event.* flags.
The implementation uses a new MultiEventManager type which simply wraps a list of PubsubEventManagers. This approach allows for maximum code reuse with minimum code churn, although it might technically be cleaner to altogether replace the implementation of PubsubEventManager with that of MultiEventManager in the future.
Implements #2131