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

feat: add support for RDS postgres instances #25

Conversation

emmanuelnk
Copy link
Contributor

@emmanuelnk emmanuelnk commented Jun 21, 2024

This PR adds support for IDatabaseInstance as well.

Also fixes #27

@emmanuelnk emmanuelnk force-pushed the feat/add-support-rds-postgres-instance branch from 94c2e76 to be734a0 Compare June 21, 2024 22:23
@emmanuelnk emmanuelnk marked this pull request as draft June 21, 2024 22:24
@berenddeboer
Copy link
Owner

berenddeboer commented Jun 22, 2024

Thanks @emmanuelnk a lot of work went into this! But my first thought is that this should have been using a union. So there's really no need, unless you come from languages like Java, to introduce new classes.

Why can't you change the provider "cluster" property to accept an RDS instance? Your change seems much more massive than needed, although I have not spent time to investigate this.

@emmanuelnk emmanuelnk force-pushed the feat/add-support-rds-postgres-instance branch from be734a0 to 24d6e8c Compare June 23, 2024 13:03
@emmanuelnk
Copy link
Contributor Author

@berenddeboer I took in your suggestion and made it such that cluster prop accepts IDatabaseInstance interface. I've tested this via my fork and it works as expected. There are now less changes and no breaking changes to the original functionality.

@emmanuelnk emmanuelnk marked this pull request as ready for review June 23, 2024 20:26
@berenddeboer
Copy link
Owner

OK, this is looking a lot more like what I expected to see!

@emmanuelnk
Copy link
Contributor Author

@berenddeboer Any chance we can get this merged in?

@berenddeboer berenddeboer merged commit aa111d2 into berenddeboer:main Jun 28, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rollback on Delete throws undefined error
2 participants