Add ability to run commands before trying to connect

[svanharmelen]

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]

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]

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]

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:

• the context shouldn't be stored in Application, but passed from one method to another.
• I'm unsure you need to stored the context canceller in the struct either.
• you could move the signal check things in a method that create the context.
• The context could be created in main.go
• waitforport should pass the context. Avoiding to retry if the context is done.

[svanharmelen]

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]

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]

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.

[jorgerojas26]

I'm ready to merge this, but the only thing i think is missing is documentation, so, can you please create a section in the readme explaining this feature?

[ccoVeille]

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 😊

Maybe the discussions we had here, could be quoted in a dedicated issue about a possible refactoring that would make the app more idiomatic, especially the context.

The issue might be closed then,but at least there will be a track of the discussion in an issue

[jorgerojas26]

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 😊

Maybe the discussions we had here, could be quoted in a dedicated issue about a possible refactoring that would make the app more idiomatic, especially the context.

The issue might be closed then,but at least there will be a track of the discussion in an issue

That is a really good idea. I sadly did not know about Context api when i started building lazysql (remember this is my first touch on Go). So, that refactor would be a good idea.

[svanharmelen]

@jorgerojas26 just added a commit with some documentation, so guess this one should be ready now as well.