Wrap custom commands on shell

[gusandrioli]

Context

This solves #354

Implementation

Ideally, we'd re-use ICmdObjBuilder from lazygit across both projects, but that'd require a complete refactor here. So i ported the string replacement for Windows OS and the wrapping of new shell.

[gusandrioli]

Here's the result using the issue's example:

(base) ➜  lazydocker git:(master) ✗ go run -mod=vendor main.go
docker-compose config --quiet

+ bash -c docker run -it -v "$(pwd)":/data redis

1:C 15 Oct 2022 19:27:44.265 # oO0OoO0OoO0Oo Redis is starting oO0OoO0OoO0Oo
1:C 15 Oct 2022 19:27:44.265 # Redis version=7.0.5, bits=64, commit=00000000, modified=0, pid=1, just started
1:C 15 Oct 2022 19:27:44.265 # Warning: no config file specified, using the default config. In order to specify a config file use redis-server /path/to/redis.conf
1:M 15 Oct 2022 19:27:44.265 * monotonic clock: POSIX clock_gettime
                _._                                                  
           _.-``__ ''-._                                             
      _.-``    `.  `_.  ''-._           Redis 7.0.5 (00000000/0) 64 bit
  .-`` .-```.  ```\/    _.,_ ''-._                                  
 (    '      ,       .-`  | `,    )     Running in standalone mode
 |`-._`-...-` __...-.``-._|'` _.-'|     Port: 6379
 |    `-._   `._    /     _.-'    |     PID: 1
  `-._    `-._  `-./  _.-'    _.-'                                   
 |`-._`-._    `-.__.-'    _.-'_.-'|                                  
 |    `-._`-._        _.-'_.-'    |           https://redis.io       
  `-._    `-._`-.__.-'_.-'    _.-'                                   
 |`-._`-._    `-.__.-'    _.-'_.-'|                                  
 |    `-._`-._        _.-'_.-'    |                                  
  `-._    `-._`-.__.-'_.-'    _.-'                                   
      `-._    `-.__.-'    _.-'                                       
          `-._        _.-'                                           
              `-.__.-'                                               

1:M 15 Oct 2022 19:27:44.266 # Server initialized
1:M 15 Oct 2022 19:27:44.269 * Ready to accept connections
^C1:signal-handler (1665862067) Received SIGINT scheduling shutdown...
1:M 15 Oct 2022 19:27:47.999 # User requested shutdown...
1:M 15 Oct 2022 19:27:47.999 * Saving the final RDB snapshot before exiting.
1:M 15 Oct 2022 19:27:48.022 * DB saved on disk
1:M 15 Oct 2022 19:27:48.023 # Redis is now ready to exit, bye bye...


Press enter to return to lazydocker (this prompt can be disabled in your config by setting `gui.returnImmediately: true`)

[gusandrioli]

@jesseduffield This is ready, let me know your thoughts!

[jesseduffield]

Looking good, one comment.

Also we should remove RunDirectCommand given that's not being used and seemed to do something similar

[jesseduffield]

Also one thing we'll want to test @gusandrioli is whether an existing custom command which already used bash -c explicitly now breaks with this new code

[gusandrioli]

Also one thing we'll want to test @gusandrioli is whether an existing custom command which already used bash -c explicitly now breaks with this new code

Good point, i'll test that out.

[gusandrioli]

@jesseduffield It does fail! Thanks for catching that:

+ bash -c bash -c docker run -it -v "$(pwd)":/data redis


Usage:  docker [OPTIONS] COMMAND

A self-sufficient runtime for containers

One alternative is to leave the command as is if it contains bash -c . What are your thoughts on this?

[gusandrioli]

@jesseduffield Updated the config to have a invokeShell bool (default: false). Tested on both cases and worked fine.

Also, for these type of PRs with a lot of commits, do you prefer if i squash them or leave as is?

[jesseduffield]

couple things

[jesseduffield]

rather than mutate option.command here I would have a separate resolvedCommand variable that we use. This means that if we want to reuse options from outside this callback down the line there's no chance of the callback's code interfering

[gusandrioli]

I see. Okay, makes sense.

[gusandrioli]

@jesseduffield Fixed :) Thanks for the feedback.

[jesseduffield]

Nice work :) I'll give this a local test soon

[gusandrioli]

Hey @jesseduffield! Hope you're doing well. Are we still going to use this PR? If so, i can solve the conflicts.

[jesseduffield]

@gusandrioli sorry this fell through the cracks. I'll take another look this afternoon

[gusandrioli]

@jesseduffield No worries at all, let me know :)

[jesseduffield]

Code looks good, I've just fixed some merge conflicts

[jesseduffield]

can you confirm this still works on your end after I resolved those merge conflicts @gusandrioli ?

[gusandrioli]

Hey @jesseduffield, just tested and it's working. Thanks for solving the conflicts!

Testing Results

Using the following config

customCommands:
  images:
    - name: tryout
      attach: true
      shell: false
      command: bash -c "docker run -it -v "$(pwd)":/data redis"
      serviceNames: []
    - name: tryout2
      attach: true
      shell: true
      command: docker run -v "$(pwd)":/data redis
      serviceNames: []

For tryout:

And for tryout2:

[jesseduffield]

Merged. Thanks @gusandrioli !

[mark2185]

This broke the custom commands like these:

customCommands:
  containers:
    - name: bash
      attach: true
      command: 'docker exec -it {{ .Container.ID }} bash'
      serviceNames: []

Results with the error:

+ bash -c docker exec -it {{ .Container.ID }} bash

Error response from daemon: No such container: {{

Setting shell: false changes nothing.

[VaultVulp]

This changeset broke lazydocker completely for me. I hope the issue will be resolved, ASAP.

For now I've downgrade to 0.20.0 via homebrew using the following snippet:

wget https://raw.githubusercontent.com/jesseduffield/homebrew-lazydocker/b6d29bcd59058d1b7ec371df12f75d7048b40789/lazydocker.rb
brew uninstall lazydocker
brew install -s lazydocker.rb
rm ./lazydocker.rb