-
Notifications
You must be signed in to change notification settings - Fork 216
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
Add event trigger resource management #463
base: main
Are you sure you want to change the base?
Conversation
f2c2e47
to
dea1401
Compare
@cyrilgdn I've used this to add a few hundred event triggers where I work, our fork has a few more commits with some fixes of things that we found after opening the PR, I can push the fixes to this branch but it's not clear to me if you're interested in that, let me know either way. |
Hi @Fabianoshz , Thanks for your work and for opening this PR, |
Perfect, I take some time this week to push the other fixes we've added on our fork |
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.
Thanks again for you work, here are some comments. Basically:
- The ID used in
setID
needs to be based on database name / event name and not only event name - The update function doesn't work well for some cases, I explained how to test it easily in the TestCase directly
- The Exists function can be removed
- A small bug in the read function for the owner
- and a few minor comments
let me know if you need help to fix that.
b = bytes.NewBufferString("ALTER EVENT TRIGGER ") | ||
fmt.Fprint(b, pq.QuoteIdentifier(eventTriggerName)) | ||
|
||
eventTriggerEnabled := d.Get(eventTriggerStatusAttr).(string) |
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.
Does it work for enable_replica
and enable_always
?
it seems to generate ALTER EVENT TRIGGER name enable_replica
where the documentation says:
ALTER EVENT TRIGGER name ENABLE [ REPLICA | ALWAYS ]
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 actually checked with your test by adding an extra step like:
{
Config: fmt.Sprintf(testAccPostgreSQLEventTriggerConfig, dbName, schemas[0], "enable"),
Check: resource.ComposeTestCheckFunc(
testAccCheckPostgresqlEventTriggerExists("postgresql_event_trigger.event_trigger", dbName),
),
},
{
Config: fmt.Sprintf(testAccPostgreSQLEventTriggerConfig, dbName, schemas[0], "enable_always"),
Check: resource.ComposeTestCheckFunc(
testAccCheckPostgresqlEventTriggerExists("postgresql_event_trigger.event_trigger", dbName),
),
},
(and changing the conf definition to take %[3]s
instead of "enabled" directly)
so it tries to alter the status of the event and it indeed fails with:
Error: pq: syntax error at or near "enable_always"
Full PG logs being:
ERROR: syntax error at or near "enable_always" at character 34
STATEMENT: ALTER EVENT TRIGGER "event_trigger_test" enable_always
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've added a map to fix this
return nil | ||
} | ||
|
||
func resourcePostgreSQLEventTriggerUpdate(db *DBConnection, d *schema.ResourceData) error { |
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.
You don't manage the renaming of an event properly, similar to the comment on status
you can test easily by updating your test case with a new step that rename it, e.g.:
{
Config: fmt.Sprintf(testAccPostgreSQLEventTriggerConfig, dbName, schemas[0], "test_event"),
Check: resource.ComposeTestCheckFunc(
testAccCheckPostgresqlEventTriggerExists("postgresql_event_trigger.event_trigger", dbName),
),
},
{
Config: fmt.Sprintf(testAccPostgreSQLEventTriggerConfig, dbName, schemas[0], "test_event_renamed"),
Check: resource.ComposeTestCheckFunc(
testAccCheckPostgresqlEventTriggerExists("postgresql_event_trigger.event_trigger", dbName),
),
},
In this case, this test will fail with:
Error: pq: event trigger "test_event_renamed" does not exist
You can simply check here if the name changed with something like:
if d.HasChange(eventTriggerNameAttr) {
old, new := d.GetChange(eventTriggerNameAttr)
if _, err := txn.Exec(
fmt.Sprintf("ALTER EVENT TRIGGER %s RENAME TO %s", pq.QuoteIdentifier(old), pq.QuoteIdentifier(new)),
); err != nil {
return err
}
}
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've just added a rename, I've also added your step to the tests too.
@cyrilgdn thanks for throughout review, I will start working on fixing in the next few days |
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.
@cyrilgdn sorry for the delay, I've addressed your requests, let me know if you think I should change anything, I will also test this on my environment in the meantime.
return nil | ||
} | ||
|
||
func resourcePostgreSQLEventTriggerUpdate(db *DBConnection, d *schema.ResourceData) error { |
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've just added a rename, I've also added your step to the tests too.
b = bytes.NewBufferString("ALTER EVENT TRIGGER ") | ||
fmt.Fprint(b, pq.QuoteIdentifier(eventTriggerName)) | ||
|
||
eventTriggerEnabled := d.Get(eventTriggerStatusAttr).(string) |
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've added a map to fix this
My organization would also benefit from this feature! @Fabianoshz really appreciate your contribution here. |
I'm opening this as a draft in case anyone wants to give feedback or test the code.
This should allow us to manage event triggers in this provider, here's how the resource would look like:
Closes #398