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

Update psql drop_command to force dropdb #81

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Andsbf
Copy link
Contributor

@Andsbf Andsbf commented Apr 4, 2023

Context:

The reason behind it is to improve developer experience, as right now when running a command like hanami db reset it will fail if the user has any other ongoing connection, like a connection to db client(like dbeaver), or a hanami console session.

image

Couple other possible approaches are:

              # create a dedicated method for force drop
              def force_drop_command
                system(cli_env_vars, "dropdb --force #{escaped_name}")
              end
              # force as an option/param
              def drop_command(force=false)
              command_prefix =  force ? "dropdb --force" : "dropdb"
              
                system(cli_env_vars, "#{command_prefix} #{escaped_name}")
              end

Reference:
https://www.postgresql.org/docs/current/sql-dropdatabase.html#id-1.9.3.108.6

The reason behind it is to improve developer experience, as right now when
running a command like `hanami db reset` it will fail if the user has any other
ongoing connection, like a connection to db client(like dbeaver), or a `hanami
console` session.
@parndt
Copy link
Contributor

parndt commented Apr 24, 2023

I don't think we should use force by default, but making it an option would be 👍

@timriley
Copy link
Member

Hi @Andsbf! Thanks for this confibution :) Like @parndt suggested, I think this would work best as a --force option to the db drop command, rather than it being the default.

Would you mind adjusting this PR to turn it into an option? Thanks!

Give the DB drop command the option to force drop the Database
def drop_command
system(cli_env_vars, "dropdb #{escaped_name}")
def drop_command(force: false)
command = force ? "dropdb --force" : "dropdb"
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'm not sure what would be your favourite flavour here this or that:

                command = "dropdb"

                if force
                  command << " --force"
                end

@@ -17,7 +17,7 @@ def create_command
end

# @api private
def drop_command
def drop_command(**)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems like "unlink" doesn't support any params, so nothing to do here
https://apidock.com/ruby/Pathname/unlink

@Andsbf
Copy link
Contributor Author

Andsbf commented Jun 29, 2023

@parndt @timriley thanks for the follow up, I have updated the PR according to the conversation above, let me know if I got it right. I could not find document to update regard that interface change (making the Hanami::CLI::Commands::App::DB::Drop) take an optional value, so let me know if I miss it.

I would appreciate some help here on how to use it....given I have the following on my hanami file:
image

if this PR gets merged I would update my "db drop" register command to this?

Hanami::CLI.register "db drop", Hanami::CLI::Commands::App::DB::Drop.new(force: true)

also would the other commands "follow" on the "force" ? I mean, if I call "db reset" would it use the "db drop" with the force: true ? looking at this file I would assume it wouldn't:

https://github.com/hanami/cli/blob/main/lib/hanami/cli/commands/app/db/reset.rb

I highly appreciate you guys putting the time to look at this PR, thanks again!

@Andsbf Andsbf requested a review from parndt June 29, 2023 03:09
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.

3 participants