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

Fixes an issue with S3 notifications #1487

Merged
merged 1 commit into from
Sep 30, 2022
Merged

Fixes an issue with S3 notifications #1487

merged 1 commit into from
Sep 30, 2022

Conversation

stunthamster
Copy link
Contributor

Signed-off-by: Michael Duffy mike@stunthamster.com

Description of your changes

This PR fixes an issue where SNS notifications cannot be added to S3 buckets using topicRef names. Currently, the topicRef attempts to find a SNSTopic object from the deprecated notification.aws.crossplane.io API group. This PR changes it to use the correct SNS API group.

I have:

  • [ x] Read and followed Crossplane's contribution process.
  • [ x] Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

N/A

…try to locate a topic created with the deprecated Notifications API Group. This commit amends it to use the current SNS API Group

Signed-off-by: Michael Duffy <mike@stunthamster.com>
Copy link
Member

@haarchri haarchri left a comment

Choose a reason for hiding this comment

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

LGTM

@haarchri
Copy link
Member

haarchri commented Sep 21, 2022

@muvaf can we merge it ? How do you feel about it because of deprecation of notification? we will hold the next release v0.32.0 till we make a discission

@nabuskey
Copy link
Contributor

With this change, existing resources using SNS notification will not be affected as long as resolution policy is set to IfNotPresent. New resources, however, will not have references to the SNS topic resource. So composition for that will need to be updated.

Looks like SNSTopic was marked deprecated 8 months ago. Is there a timeline for removing this api group?

@haarchri
Copy link
Member

i am fine to say in v0.33.0 to make this change and remove the old apiGroup - depends on the others =)

@haarchri
Copy link
Member

we will remove with provider release v0.33.0 at Oct 19, 2022 the depcrecated notification APIGroup - and will merge this PR !!!
#1502

@haarchri haarchri merged commit 03a6daf into crossplane-contrib:master Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants