-
Notifications
You must be signed in to change notification settings - Fork 434
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
Feature/552 eventsubscription #928
Feature/552 eventsubscription #928
Conversation
v3.0.20 release 🚀
v3.1.0 release 🚀
This is a wip commit with some todo question items. I would like to do a rebase before opening the PR, so that I send in one clean commit to the pull request. Also, there is another todo item: I should make the needed changes to go.mod, but no other changes.
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.
Hey @datadeverik, thanks for the PR
I've left some minor comments
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
}) | ||
} | ||
} | ||
|
||
if aws.ToString(output.Marker) == "" { // TODO: is output.Marker here playing the same role as ouput.NextMarker in the efs example? | ||
if aws.ToString(output.Marker) == "" { |
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.
Should we add a logger? for debugging purposes?
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'll look around for examples of loggers in a similar positions in existing functions.
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.
After some basic inspection, I don't see a logger in similar places in existing functions in the AWS provider. (like for example providers/aws/lambda/functions.go
).
But now it occurs to me that maybe you were suggesting adding a logger temporarily to answer the question in the comment, namely "TODO: is output.Marker here playing the same role as ouput.NextMarker in the efs example?".
In any case, it seems that the feature is working correctly, so maybe there isn't a need to pursue this point further.
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.
Sounds good to me I think we are ready to go with this!
Problem
Fixes #552
Solution
Add eventsubscription
Changes Made
Add support for Amazon Redshift eventsubscription
How to Test
I created an eventSubscription through the awscli, then checked that it appeared in the Komiser UI.
Screenshots
Notes
I indicated in comments in the code some points I'm not sure about. Also, testing and documentation isn't done, but I wanted to get some eyes on what I've done so far. Also, I used
go get go get github.com/aws/aws-sdk-go-v2/service/redshift
, which I noticed changed various version numbers. I'm not sure if this is what we want at this point.Checklist
Reviewers
@[username of the reviewer]