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 resource for subscriptions #244

Merged
merged 9 commits into from
Nov 26, 2022

Conversation

nicarl
Copy link
Contributor

@nicarl nicarl commented Aug 28, 2022

This PR adds the new resource postgresql_publication for managing PostgreSQL publications.

Example:

resource "postgresql_subscription" "subscription" {
  name          = "subscription"
  conninfo      = "host=localhost port=5432 dbname=mydb user=postgres password=secret"
  publications  = ["publication"]
}

Key decisions:

  • I decided to let the user pass conninfo as one string instead of adding fields for host, port, etc.
  • There are additional options when setting up a subscription (https://www.postgresql.org/docs/current/sql-createsubscription.html), I did not add support for those yet to keep this as a small as possbile
  • I could not find a reliable way to update individual parameters - destroy and recreate is therefore preferred
  • For the testing one need to set the create_slot to false as the publisher is on the same database cluster. Otherwise the CREATE statement will hang. This is a known issue (https://www.postgresql.org/docs/current/sql-createsubscription.html)
  • The tests get flaky when not waiting a few seconds after the test. This seems to be related to the replication slots. I choose a pragmatic solution here and added a cool down period of 5 seconds

fix: #238

@nicarl
Copy link
Contributor Author

nicarl commented Aug 28, 2022

cc @chromko maybe you can have a look at this as you added the publication resource?

@nicarl nicarl force-pushed the feat/add-subscriptions branch from b8ddae9 to 7d3d728 Compare August 29, 2022 16:37
@nicarl
Copy link
Contributor Author

nicarl commented Aug 30, 2022

Hey @cyrilgdn I ran the tests successfully on my local machine, I would be curious if they pass in the CI

@cyrilgdn
Copy link
Owner

@nicarl Thanks for you work on that 👍

I would be curious if they pass in the CI

Just triggered them!

I'll try to review as soon as possible (hopefully this week-end)

d.Set("database", databaseName)
d.SetId(generateSubscriptionID(d, databaseName))

createSlot, okCreate := d.GetOkExists("create_slot") //nolint:staticcheck
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicarl
Copy link
Contributor Author

nicarl commented Sep 6, 2022

@nicarl Thanks for you work on that 👍

I would be curious if they pass in the CI

Just triggered them!

I'll try to review as soon as possible (hopefully this week-end)

Hey, did you have time to look at this?

@nicarl
Copy link
Contributor Author

nicarl commented Oct 18, 2022

Hey @cyrilgdn the tests are passing, did you have any time to look at the PR?

The password is part of the connection string and will be outputed to
logs in case of an error.

Remove it in case it's present.
@nicarl nicarl force-pushed the feat/add-subscriptions branch from 7faa84d to 70f2d5d Compare October 24, 2022 07:41
@nicarl nicarl force-pushed the feat/add-subscriptions branch from a017791 to 78df550 Compare October 24, 2022 07:51
@cyrilgdn
Copy link
Owner

Sorry for the absence of response, I was a bit off the project for a while due to lack of time.

I'll review it so we can include in the next release, could you just merge master to resolve the conflict (you can revert test.yml to the master version)

@cyrilgdn cyrilgdn self-requested a review November 20, 2022 13:44
postgresql/config.go Outdated Show resolved Hide resolved
Copy link
Owner

@cyrilgdn cyrilgdn left a comment

Choose a reason for hiding this comment

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

Thanks for your work 👍

@cyrilgdn cyrilgdn merged commit b8e4870 into cyrilgdn:master Nov 26, 2022
@cyrilgdn
Copy link
Owner

This has been released in v1.18.0

@nicarl
Copy link
Contributor Author

nicarl commented Nov 28, 2022

Hey, sorry I did not find time earlier. Thank you for fixing the last details and your work for this provider! 🙇

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.

Feature request: Subscriptions
3 participants