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

fix typo in leadership resignation query #134

Merged
merged 1 commit into from
Dec 31, 2023

Conversation

bgentry
Copy link
Contributor

@bgentry bgentry commented Dec 30, 2023

s/river_leaderhip/river_leadership 🤦‍♂️

@bgentry bgentry requested a review from brandur December 30, 2023 21:55
@bgentry bgentry marked this pull request as ready for review December 30, 2023 21:55
@bgentry bgentry force-pushed the bg-fix-leadership-notify-name branch from eef6cd5 to 789610f Compare December 30, 2023 21:56
@@ -40,7 +40,7 @@ WITH currently_held_leaders AS (
),
notified_resignations AS (
SELECT
pg_notify('river_leaderhip', json_build_object('name', name, 'leader_id', leader_id, 'action', 'resigned')::text),
pg_notify('river_leadership', json_build_object('name', name, 'leader_id', leader_id, 'action', 'resigned')::text),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, another way to fix this would be to pass in the notification name using the in-code constant. That would ensure we're using the same value everywhere.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, good idea. It's already referenced in code anyway, so couldn't hurt just to use the same ref here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll do this in a follow up PR, there are a couple we can get rid of

Copy link
Contributor

@brandur brandur left a comment

Choose a reason for hiding this comment

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

Doh! LGTM.

@@ -40,7 +40,7 @@ WITH currently_held_leaders AS (
),
notified_resignations AS (
SELECT
pg_notify('river_leaderhip', json_build_object('name', name, 'leader_id', leader_id, 'action', 'resigned')::text),
pg_notify('river_leadership', json_build_object('name', name, 'leader_id', leader_id, 'action', 'resigned')::text),
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, good idea. It's already referenced in code anyway, so couldn't hurt just to use the same ref here.

@bgentry bgentry merged commit 7392231 into master Dec 31, 2023
7 checks passed
@bgentry bgentry deleted the bg-fix-leadership-notify-name branch December 31, 2023 01:19
bgentry added a commit that referenced this pull request Dec 31, 2023
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.

2 participants