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

Use Laravel relationships to remove configure and note rows #2613

Merged
merged 1 commit into from
Dec 13, 2024
Merged

Use Laravel relationships to remove configure and note rows #2613

merged 1 commit into from
Dec 13, 2024

Conversation

zackgalbreath
Copy link
Contributor

@zackgalbreath zackgalbreath commented Dec 3, 2024 •
edited
Loading

Replace custom SQL with Laravel relationships to identify configure and note rows that are only being used by builds that are about to be deleted.

@zackgalbreath zackgalbreath marked this pull request as draft December 3, 2024 21:36
williamjallen
williamjallen requested changes Dec 3, 2024
View reviewed changes
@williamjallen williamjallen added this to the v3.7 milestone Dec 3, 2024
@zackgalbreath zackgalbreath force-pushed the remove_configure_by_relationship branch 2 times, most recently from 2c11b67 to 965ce5a Compare December 12, 2024 19:28
@zackgalbreath zackgalbreath marked this pull request as ready for review December 12, 2024 19:51
@zackgalbreath zackgalbreath changed the title Use Laravel relationships to remove configure rows Use Laravel relationships to remove configure and note rows Dec 12, 2024
williamjallen
williamjallen reviewed Dec 13, 2024
View reviewed changes
@zackgalbreath
Use Laravel relationships to remove configure and note rows
d01f913 
Replace custom SQL with Laravel relationships to identify configure
and note rows that are only being used by builds that are about to be deleted.
@zackgalbreath zackgalbreath force-pushed the remove_configure_by_relationship branch from 965ce5a to d01f913 Compare December 13, 2024 16:48
williamjallen
williamjallen approved these changes Dec 13, 2024
View reviewed changes
Copy link
Collaborator

@williamjallen williamjallen left a comment

Choose a reason for hiding this comment

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

Code looks good to me, and the new query plan seems reasonable. Thanks for taking care of this! It's great that we can delete so much complicated logic in favor of a single query constructed by Eloquent.

@zackgalbreath zackgalbreath added this pull request to the merge queue Dec 13, 2024
Merged via the queue into master with commit ca615cd Dec 13, 2024
6 checks passed
@williamjallen williamjallen deleted the remove_configure_by_relationship branch December 13, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@williamjallen williamjallen williamjallen approved these changes

Assignees
No one assigned
Labels
release notes: performance
Projects
None yet
Milestone
v3.7
Development

Successfully merging this pull request may close these issues.
2 participants
@zackgalbreath @williamjallen