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

Stop using LockCol in registrations table #7935

Merged
merged 5 commits into from
Jan 17, 2025
Merged

Stop using LockCol in registrations table #7935

merged 5 commits into from
Jan 17, 2025

Conversation

jprenken
Copy link
Contributor

@jprenken jprenken commented Jan 11, 2025 •
edited
Loading

Alter the LockCol column to have a default value, so we can omit it from INSERTs.

Part of #7934

@jprenken jprenken changed the base branch from main to updatereg-3 January 13, 2025 18:57
@jprenken jprenken marked this pull request as ready for review January 13, 2025 18:58
@jprenken jprenken requested a review from a team as a code owner January 13, 2025 18:58
@jprenken jprenken requested review from beautifulentropy and removed request for a team January 13, 2025 18:58
@github-actions GitHub Actions
Copy link
Contributor

@jprenken, this PR appears to contain configuration and/or SQL schema changes. Please ensure that a corresponding deployment ticket has been filed with the new values.

@letsencrypt letsencrypt deleted a comment from github-actions bot Jan 13, 2025
@beautifulentropy
Copy link
Member

Taking into account the changes in #7911, this generally looks good. I'll take another look once #7911 is merged.

Base automatically changed from updatereg-3 to main January 14, 2025 21:54
@jprenken
Copy link
Contributor Author

IN-10959

@jprenken jprenken mentioned this pull request Jan 15, 2025
Open
beautifulentropy
beautifulentropy approved these changes Jan 15, 2025
View reviewed changes
Copy link
Member

@beautifulentropy beautifulentropy left a comment

Choose a reason for hiding this comment

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

This looks great. A note for other reviewers: the first time I looked at this migration the ALTER COLUMN syntax caught me by surprise. I'm used to seeing MODIFY COLUMN from MySQL. But since at least MariaDB 10.2 this has been part of the broader ALTER TABLE functionality

@beautifulentropy beautifulentropy requested review from a team, jsha and aarongable and removed request for a team January 15, 2025 20:16
aarongable
aarongable approved these changes Jan 17, 2025
View reviewed changes
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

LGTM. The LockCol that used to be on the certificateStatus table had a DEFAULT 0 before we dropped it, so this makes sense as the correct step towards dropping this column entirely.

@jprenken jprenken merged commit 6f4eb5a into main Jan 17, 2025
15 checks passed
@jprenken jprenken deleted the unlockcol-1 branch January 17, 2025 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aarongable aarongable aarongable approved these changes

@beautifulentropy beautifulentropy beautifulentropy approved these changes

@jsha jsha Awaiting requested review from jsha jsha is a code owner automatically assigned from letsencrypt/boulder-developers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jprenken @beautifulentropy @aarongable