Executing "run" no longer works to forward to other commands

[Neunerlei]

Description

Hello there,

first of all thanks for creating this awesome tool, which I use basically in every project I work on.

I wrote quite a few cli tools running with bashly and really loved the possibility to create a command that forwards to another command if needed.

E.g. I have a environment control script with a npm install command, which first should execute the docker up command. Until now I did this by calling run docker up, followed by a run docker exec npm install in the npm install command.

Yes, I know I was using internals here, but I found it to be an amazing tool to reuse commands in an atomic pattern that did not involve rebooting the whole script over and over.

However, the latest change: 9167c49

breaks this behavior, because input and args are no longer reset, which currently breaks all scripts.
I'm currently considering my options here and wanted to ask if you could point me in a direction to fix this.
I'm currently thinking about creating a custom "forward_to" function in my library code which restores the old variable resetting, but that will be a lot of refactoring. 🤔

Alternatively I would like to kindly ask if you would consider rolling back the change to the previous behavior?

Thank you in any way!

edit: here is a bit of source code as context:

bashly.yaml

name: env
help: Environment Control application
version: 1.0.0
commands:
  - name: up
    help: similar to docker-compose up, but sets all the required environment
      variables for buildkit. (All docker command options are valid)
    catch_all: true
    flags:
      - long: --attach
        short: -f
        help: By default, the containers are started daemonized, use this to attach to
          their output
  - name: ssh
    alias: shell
    help: basically your docker-compose exec into the main app container
    args:
      - name: service
        help: Allows you to define which service you want to connect to
    flags:
      - long: --cmd
        short: -c
        arg: command
        default: bash
        help: By default, we will use "bash" as command, you can use "sh" or any other
          command if you want to
   - name: composer
    help: runs a certain composer command for the project
    catch_all: true         

up_command.sh

ARGS=${other_args[*]}

if ! [ ${args[--attach]} ]; then
	ARGS+=" -d"
fi

$DOCKER_COMPOSE_EXECUTABLE up $ARGS

ssh_command.sh

SERVICE=${args[service]:-$DEFAULT_SERVICE_NAME}
CMD=${args[--cmd]:-bash}

if ! isDockerComposeServiceRunning; then
  run up
fi
CONTAINER_ID=$(getContainerIdFromServiceName $SERVICE)

$DOCKER_EXECUTABLE \
  exec -ti \
  ${CONTAINER_ID} \
  bash -c "${CMD}"

composer_command.sh

CMD="${other_args[*]}"

run ssh -c "composer ${CMD}"

[DannyBen]

Hi and thank you for this issue.
Apologies of course for creating a breaking change - we go through excessive testing to ensure no "declared" behavior is broken, but as you mentioned, you used something internal.

However - I do not want this latest change to break anything, but I am not sure yet I fully understand why it breaks. I will need to dig deeper into it.

For now, I suggest this:

1. You roll back to the previous version until we figure out how to resolve this without you having to change your scripts.
2. If you can review your code snippet above, and just distill it to its most minimal form including bashly.yml, command partials (if needed) and commands to execute with expected vs actual results - it will help me to zoom in on the problem.

---

I have looked at the change and I am a bit relieved - I thought it was related to the #596.
Rolling back this change is entirely an option. I will take a deeper look.

[Neunerlei]

Hey there!

Thanks for your fast reply :)

The "issue" seems to be, that, because the "input", "args", etc. variables are no longer reset when the "run" function is executed, it basically appends the new input to the existing one.

So the original "input" in run is for example "composer", then I call run ssh, which until now created an input of "ssh",
but because the vars are now only reset in the "inizialize" part, everything is just appended, leading to an input of "composer ssh" instead, leading to the composer command being executed again, calling run ssh, creating "composer ssh ssh" and so on. Which will then create an infinite loop (which creates a nice pattern in the cli, btw :D

)

What I did to work around that was to create a wrapper.

runSubCommand() {
  declare -g -A args=()
  declare -g -a other_args=()
  declare -g -A deps=()
  declare -g -a env_var_names=()
  declare -g -a input=()

  run "$@"
}

which I use instead of calling run directly, works fine for now, but is quite hacky of course.
It would be really cool if that "call another command" thingy was some kind of a feature instead of me abusing your internals (shame on me) 🙈

[DannyBen]

What I did to work around that was to create a wrapper.

Yeah, no - I do not like it either.

For some reason I thought you were calling the commands themselves (docker_command()) but then realized you were using a use case I have never seen before.

It is nice to just be able to call "run ..." and have the script just execute as if it was called externally, but it was definitely not designed to do this - as you notice.

If we make any change to support this - then it will be official, formal, and fully tested for future regression.
We just need to decide if this is a good idea - since this could prove to be a limiting functionality.

Personally - I usually avoid such pattern, and instead, create my functions in the lib directory, and then any other function can call them as isolated units, completely bypassing the CLI entrypoint. Is that something that you relate to?

[Neunerlei]

Yeah, that is what I will probably do in the next couple of days.
This is why I created it as "feature request" not as a "bug", because it is only my niche use-case that broke 🤷‍♂️

Thanks for your feedback and have a nice day :)

P.S. You may close this if you like

[DannyBen]

No, keep it open. If it doesn't behave as you expect, I consider it at least a pseudo-bug.
I want to look into it, see if we can make it a feature.

[DannyBen]

@Neunerlei see the associated PR - I am reverting (and slightly improving) the change, since I did not think about this nice use case of yours when I made it. I am beginning to wonder if there is any need for initialize at all, but I will leave this be for not.

I still need to complete some tests and documentation for it, but you can consider it reverted.

It would help if you can test this unreleased version, to ensure your use case is indeed back in the green.

Thanks for the ticket and for your support.

[Neunerlei]

@DannyBen Thank you very much 👍 This is awesome ❤️

[DannyBen]

@Neunerlei - version 1.2.8 (changelog) is released with this change.
Thanks for raising this issue. I am going to close it now, but feel free to comment here or open a new issue with any problem or question.