-
Notifications
You must be signed in to change notification settings - Fork 1
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
[ETL-553] Allow merging of notification configs #87
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. I left some comments but I'm approving this as is.
I realized we've created a new problem for ourselves by accumulating bucket event notification rules. This is a super minor issue, but I've filed a ticket here to handle that a bit more elegantly. Maybe we can piggyback these changes on some other work that needs to be done: https://sagebionetworks.jira.com/browse/ETL-572
It's also worth considering why this lambda ought to be namespaced at all. Why not make it a general use lambda that is invoked by all our branches? The current implementation is perfectly functional so I don't think this is a particularly high priority change, either.
@@ -40,14 +48,111 @@ def lambda_handler(event, context): | |||
raise KeyError(err_msg) | |||
|
|||
|
|||
def get_existing_bucket_notification_configuration_and_type( | |||
s3_client: boto3.client, bucket: str, destination_type: str | |||
) -> typing.Tuple[dict, list]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be more readable as a named tuple or dict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this to: b473c56 - Is this what you had in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works! I oftentimes use dict to return multiple results in a clear way but the way typing works with a class is pretty nice.
if index_of_matching_arn is not None: | ||
if json.dumps( | ||
matching_notification_configuration, sort_keys=True | ||
) != json.dumps(new_notification_configuration, sort_keys=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think it would be cleaner to compare the "index" of these two notifications configurations, the index being the necessary properties Events
and Filter.Key.FilterRules.Value
in our notification configuration. I think it's less likely that these fields will change in the future versus some arbitrary change in the response. But this works for now 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this thought. I will extract this out to a compare
function so all the fields we are checking is in one spot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this to: b473c56
destination_type_arn: str, | ||
existing_notification_configurations_for_type: list, | ||
destination_arn: str, | ||
) -> typing.Union[tuple[int, dict], tuple[None, None]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, this would be more readable as a named tuple or dict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this in f7c2e3a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥 LGTM!
@@ -40,14 +48,111 @@ def lambda_handler(event, context): | |||
raise KeyError(err_msg) | |||
|
|||
|
|||
def get_existing_bucket_notification_configuration_and_type( | |||
s3_client: boto3.client, bucket: str, destination_type: str | |||
) -> typing.Tuple[dict, list]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@BryanFauble feel free to merge whenever you're comfortable with the changes. The PR is already approved. |
Problem:
s3_event_config
new notifications could not be added to an S3 bucket, instead the notification configuration was always overwritten.Solution:
A) Adding notifications:
1) If a bucket has no
NotificationConfiguration
then create the config2) If a bucket has a
NotificationConfiguration
but no matching"{destination_arn}" for the "{destination_type}" then add the config
3) If a bucket has a
NotificationConfiguration
and a matching"{destination_arn}" for the "{destination_type}":
3a) If the config is the same then do nothing
3b) If the config is different then overwrite the matching config
B) Deleting notifications - Only deleting the specific notificationConfiguration that matches the
"{destination_type}Configurations"
and destination arnTesting:
I wrote unit tests for all paths