fix: connect to template1 when database is postgres #4321
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ten0
reviewed
Oct 24, 2024
diesel_cli/src/database.rs
Outdated
| @@ -429,6 +429,11 @@ fn change_database_of_url( | |||
| .last() | |||
| .expect("The database url has at least one path segment") | |||
| .to_owned(); | |||
|
|
|||
| if database.eq("postgres") { | |||
There was a problem hiding this comment.
I've read the issue and I understand it may be sensible to do this specifically when we are trying to drop a database.
It also sounds likely that at the moment this is the only place at which this function is called.
However the naming of this function fn change_database_of_url( would not let any future code author expect this behavior, so it's likely to introduce a bug.
=> It looks like it would be best to put this logic where the function is called, that is, in a place where the current code path is about removing the database.
There was a problem hiding this comment.
Thanks for the review! I get what you mean, will push a commit with the required change.
added 4 commits
October 25, 2024 11:11
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #4275.
Description
Diesel explicitly connected to
postgresto drop any other database, which posed issues whenpostgresitself was to be dropped.Changes
Added a check for the current database, and connect to
template1for the case when current database ispostgres.