Refactor BaseDatabase class for simplicity #945
Conversation
The configuration is an instance level setting that is never passed in. This simplifies the code by avoiding the need to pass it around.
This avoids needing to use a shell.
ekohl
force-pushed
the
clean-up-base-db
branch
2 times, most recently
from
d3d4932
to
9bed8ed
Compare
| 'PGHOST' => configuration.fetch('host', 'localhost'), | ||
| 'PGPORT' => configuration.fetch('port', '5432'), | ||
| 'PGUSER' => configuration.fetch('username'), | ||
| 'PGPASSWORD' => configuration[%(password)], |
There was a problem hiding this comment.
| 'PGPASSWORD' => configuration[%(password)], | |
| 'PGPASSWORD' => configuration['password'], |
There was a problem hiding this comment.
I kept that from the previous implementation, but I agree that if we focus on simplicity that we should also apply it here.
There was a problem hiding this comment.
There it was just to escape (lol) quoting hell because of
"PGPASSWORD='#{config[%(password)]}' "
Here it's just weird :)
| def base_env | ||
| { | ||
| 'PGHOST' => configuration.fetch('host', 'localhost'), | ||
| 'PGPORT' => configuration.fetch('port', '5432'), |
There was a problem hiding this comment.
| 'PGPORT' => configuration.fetch('port', '5432'), | |
| 'PGPORT' => configuration.fetch('port', '5432').to_s, |
the port is technically an integer, and depending on where we get it from (and how), it might actually be one, but env as received by Open3 needs to be strings only.
There was a problem hiding this comment.
I now changed it so it can be nil, but now I'm not sure Open3 accepts nil values.
There was a problem hiding this comment.
nil is OK, as that means "unset"
|
I've taken this for a test-ride and it seems to behave just fine, thanks! |
This avoids the need to build a complex command. On the plus side, it avoids the risk of leaking anything about the database access. On the downside, that can make debugging harder.
This avoids the need for another helper command and simplifies the commands.
This matches other queries.
This uses a block that won't run if @deb_postgresql_versions is already set.
This wasn't used in the past for Ruby 1.9 or 2.0 support, but we can now rely on this. It makes testing easier.
ekohl
force-pushed
the
clean-up-base-db
branch
from
9bed8ed
to
50af47d
Compare
|
well, you broke your own tests, but ACK once green :) |
ekohl
force-pushed
the
clean-up-base-db
branch
from
50af47d
to
0ea3d4e
Compare
| let(:expected_env) do | ||
| { | ||
| 'PGHOST' => 'localhost', | ||
| 'PGPORT' => nil, |
There was a problem hiding this comment.
Just for my own sanity, I checked and https://rubyapi.org/3.3/o/process states:
Optional leading argument env is a hash of name/value pairs, where each name is a string and each value is a string or nil; each name/value pair is added to ENV in the new process.
This aims to make the BaseDatabase class easier to follow. At this point it's untested, but it was inspired by #939. At this point it's untested and not everything may make sense. It's structured as smaller commits that should be fairly straight forward to follow.