jorgerojas26 / lazysql

A cross-platform TUI database management tool written in Go.
MIT License
916 stars 51 forks source link

Add ability to run commands before trying to connect #129

Open svanharmelen opened 5 days ago

svanharmelen commented 5 days ago

While the code is a bit "quick and dirty", it works like a charm and allows you to define connections like this:

[[database]]
Name = 'server'
Provider = 'postgres'
DBName = 'foo'
URL = 'postgres://postgres:password@localhost:${port}/foo'
Commands = [
  { Command = 'ssh -tt remote-bastion -L ${port}:localhost:5432', WaitForPort = '${port}' }
]

So you can easily connect to remote servers as well! You can even chain commands to connect to a remote server and then a postgres container running in a remote k8s cluster:

[[database]]
Name = 'container'
Provider = 'postgres'
DBName = 'foo'
URL = 'postgres://postgres:password@localhost:${port}/foo'
Commands = [
  { Command = 'ssh -tt remote-bastion -L 6443:localhost:6443', WaitForPort = '6443' },
  { Command = 'kubectl port-forward service/postgres ${port}:5432 --kubeconfig /path/to/kube.conf', WaitForPort = '${port}' }
]
svanharmelen commented 3 days ago

I've noticed this needs a little bit more work to make it robust (does not cleanly stop all commands when having multiple connections in one session). Will make some tweaks this weekend...

svanharmelen commented 2 days ago

OK... It's quite a bit more involved as before as I needed some plumbing to be able to keep track of running commands in such a way that they would cleanup before the program exited. With these changes it now works very nicely and no matter how the app is stopped (pressing q, Ctrl-C or sending SIGINT or TERM) the commands are always properly stopped.

Next to that I added some logic for testing if a command started listening on the requested port (see the updated example in the opening post of this PR) and tried to provide a little bit more feedback to the user.

Besides the functional changes needed for the commands to work properly, I think I make 3 semi unrelated changes. If you want I can revert them, but I think they make sense:

  1. I removed the panic's from main.go
  2. I renamed loglvl to loglevel so its more consistent with logfile (which is also written in full)
  3. I updated the label on home.Tree from connection.Name+connection.Provider to just connection.Name.

The last one is mainly because practically all my names are too long for the tree width and there is a bug in how the cutoff is handled so next to loosing the last part (which makes sense) I also don't see the first few chars. I would ask/suggest to just use the connection.Name for now and maybe revert that once the behavior is fixed or changed.

Let me know if you have any questions or comments.

ccoVeille commented 2 days ago

The code is way better now thanks.

I might have some remarks on the way you use context. But there are side remarks.

It could be addressed in a later refactoring.

Here are my feedbacks:

svanharmelen commented 2 days ago

I'm well aware of how the context could/should be used in a well designed application, but properly implementing it into the current codebase would be close to a full rewrite 🙈

So I opted to not try and change everything at once, but instead focus on the bits I wanted to add and make small improvements in the surrounding parts to make that possible.

I actually started with a function that returns a context in main, more of less exactly how you are proposing 😉, but the current layout of the app is not realy designed to work that way as it uses lots of global variables to store types (like the main Application type) that are being called from locations all over the code.

Properly restructering that in a more idiomatic layout would be a reasonably big change that should be a PR on itself in my opinion (and should be endorced by @jorgerojas26 as the owner/maintainer of this code).

Edit: Not proposing such a rewrite by the way! I like the tool and love to fix bugs that bug me or contribute features I'm missing, but I do have a (quite) busy day job as well 😊

ccoVeille commented 2 days ago

I agree. That's why I said your change were OK, and the code is way better now.

And yes, it might need a pure rewrite

svanharmelen commented 2 days ago

Updated WaitForPort so it takes a context which is set on the dailer and is checked when sleeping in between connection attempts. There are still many things that can be added and/or improved regarding both the code and functionality in this PR, but for now I think this provides enough functionality for most use cases.