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.

Sign up for GitHub

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

Jump to bottom

Schema: name all constraints, not just PKs #226

Merged
merged 1 commit into from
Jul 15, 2024
Merged

Schema: name all constraints, not just PKs #226

merged 1 commit into from
Jul 15, 2024

Conversation

Al2Klimov
Copy link
Member

@Al2Klimov Al2Klimov commented Jul 2, 2024 •
edited
Loading

fixes #202

@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Jul 2, 2024
@Al2Klimov Al2Klimov marked this pull request as ready for review July 3, 2024 10:54
@Al2Klimov Al2Klimov force-pushed the schema-name-all-constraints-202 branch from 31869f0 to 5f0c38b Compare July 3, 2024 13:52
@Al2Klimov
Copy link
Member Author

Fresh schema dump matches upgraded schema dump.

incident_contact's CONSTRAINTs are already named properly.

@Al2Klimov Al2Klimov force-pushed the schema-name-all-constraints-202 branch from 5f0c38b to 65aeacf Compare July 3, 2024 13:56
julianbrost
julianbrost previously requested changes Jul 10, 2024
View reviewed changes
Copy link
Collaborator

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

  • You use a prefix for everything expect for checks where you use a _check suffix. Incremental Configuration Updates #191 started using ck_ as a prefix, it's not yet merged, but it should stay consistent, so I'd prefer a prefix, I'd be fine with both ck_* and check_*.
  • key_ doesn't sound like a good prefix for unique constraints to me. Primary and foreign keys are also keys and in MySQL/MariaDB, key actually means index (from https://dev.mysql.com/doc/refman/8.4/en/create-table.html: "KEY is normally a synonym for INDEX."). If we want to keep everything abbreviated, uk_* (unique key) might be an option, otherwise simply unique_* would also do.

schema/pgsql/schema.sql Outdated Show resolved Hide resolved
schema/pgsql/schema.sql Outdated Show resolved Hide resolved
schema/pgsql/schema.sql Outdated Show resolved Hide resolved
schema/pgsql/schema.sql Outdated Show resolved Hide resolved
@Al2Klimov Al2Klimov force-pushed the schema-name-all-constraints-202 branch from 65aeacf to dca1109 Compare July 11, 2024 15:49
@Al2Klimov Al2Klimov force-pushed the schema-name-all-constraints-202 branch from dca1109 to b9b814c Compare July 12, 2024 08:38
@Al2Klimov Al2Klimov self-assigned this Jul 12, 2024
@julianbrost julianbrost mentioned this pull request Jul 12, 2024
Merged
@Al2Klimov Al2Klimov force-pushed the schema-name-all-constraints-202 branch from b9b814c to 3fd4572 Compare July 12, 2024 09:41
@Al2Klimov Al2Klimov removed their assignment Jul 12, 2024
yhabteab
yhabteab reviewed Jul 12, 2024
View reviewed changes
schema/pgsql/schema.sql Outdated Show resolved Hide resolved
schema/pgsql/schema.sql Outdated Show resolved Hide resolved
schema/pgsql/schema.sql Outdated Show resolved Hide resolved
schema/pgsql/schema.sql Outdated Show resolved Hide resolved
schema/pgsql/schema.sql Outdated Show resolved Hide resolved
schema/pgsql/schema.sql Outdated Show resolved Hide resolved
oxzi
oxzi reviewed Jul 12, 2024
View reviewed changes
schema/pgsql/schema.sql Outdated Show resolved Hide resolved
schema/pgsql/schema.sql Outdated Show resolved Hide resolved
@Al2Klimov Al2Klimov force-pushed the schema-name-all-constraints-202 branch 3 times, most recently from 38da15f to bed578b Compare July 12, 2024 14:25
@Al2Klimov Al2Klimov requested a review from yhabteab July 12, 2024 14:25
julianbrost
julianbrost reviewed Jul 12, 2024
View reviewed changes
Copy link
Collaborator

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

Looks fine for me now from looking at the changes. If the others are happy as well, mark their comments as resolved, I'd apply it to my test setup, spin it up for a quick test and give an actual approval.

@julianbrost julianbrost dismissed their stale review July 12, 2024 14:41

The changes I requested were addressed, so this doesn't have to show up as changes requested anymore. Final approval will follow later, see #226 (review)

@Al2Klimov Al2Klimov force-pushed the schema-name-all-constraints-202 branch from bed578b to 96561ec Compare July 12, 2024 14:45
yhabteab
yhabteab reviewed Jul 12, 2024
View reviewed changes
schema/pgsql/schema.sql Outdated Show resolved Hide resolved
@Al2Klimov Al2Klimov force-pushed the schema-name-all-constraints-202 branch from 96561ec to 27aea85 Compare July 12, 2024 14:55
@Al2Klimov Al2Klimov force-pushed the schema-name-all-constraints-202 branch from 27aea85 to bcf4c31 Compare July 12, 2024 14:59
yhabteab
yhabteab approved these changes Jul 12, 2024
View reviewed changes
Copy link
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

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

LFTM!

@julianbrost julianbrost merged commit 37fb724 into main Jul 15, 2024
12 checks passed
@julianbrost julianbrost deleted the schema-name-all-constraints-202 branch July 15, 2024 08:48
@Al2Klimov Al2Klimov mentioned this pull request Jul 15, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxzi oxzi oxzi left review comments

@yhabteab yhabteab yhabteab approved these changes

@julianbrost julianbrost julianbrost approved these changes

Assignees
No one assigned
Labels
cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Schema: name all constraints, not just PKs (before v1.0)
4 participants
@Al2Klimov @julianbrost @oxzi @yhabteab