DX-3168: invokeCommand() doesn't preserve input options

[danepowell]

Motivation

Fixes #4313

invokeCommand() builds a new set of input arguments and thus discards any options such as environment.

Proposed changes

Input options need to persist through the lifecycle of a BLT process, so let's instead reuse the existing input object and modify with any new args as necessary.

[shelane]

I created a patch to run this and came up with this issue:

+ ./vendor/bin/blt artifact:deploy --commit-msg 'Dev Deployment' --branch environment-build --no-interaction
Commit message is set to Dev Deployment.
Branch is set to environment-build.
Preparing artifact directory...

Global .gitignore file is being disabled for this repository to prevent unexpected behavior.
Merging upstream changes into local artifact...

Generating build artifact...
For more detailed output, use the -v flag.
> source:build:frontend
[error]  The "--commit-msg" option does not exist. 

[danepowell]

Oh... I made sure to attach any new arguments manually specified by invokeCommand(), but I didn't realize that existing arguments attached to the input might cause problems. That is trickier... I wonder if there's any way to fix this that doesn't lead to edge cases, such as:

1. Command A takes a 'site' parameter and calls Command B
2. Command B does not take a 'site' parameter, so we drop it. Command B calls Command C
3. Command C takes a 'site' parameter, but we've now lost the site context

I guess if Command B doesn't take a particular parameter, that implies it's not important or it'll manager parameters itself. 😕

[danepowell]

This has been quite an adventure in Robo-land. I've managed to hardcode the environment parameter as context for all invoked commands. Unfortunately I don't know how to solve this for the general case, because I can't figure out how to build a new ArrayInput and detect whether an arbitrary parameter is valid for a given command. Even when I hardcode an option ($input->setOption('environment', 'test');), Robo seems to completely ignore it unless it's provided as an argument.

Accommodating --environment hopefully solves this for 90% of users, and at least won't lead to side effects.

[danepowell]

@shelane I'm going to go ahead and merge but please test and report back if you find any issues. Cheers.

[shelane]

I was running tests today with this as a patch and so far so good.

[shelane]

Yikes. Now I can't deploy a tag:

+ ./vendor/bin/blt artifact:deploy --commit-msg 'Release 9-2.0.1' --tag 9-2.0.1 --no-interaction
Commit message is set to Release 9-2.0.1.
Config option deploy.tag_source if FALSE. The source repo will not be tagged.
Tag is set to 9-2.0.1.
Preparing artifact directory...
Global .gitignore file is being disabled for this repository to prevent unexpected behavior.
Generating build artifact...
For more detailed output, use the -v flag.
> source:build:frontend
> source:build:frontend-reqs
> source:build:frontend-assets
> drupal:hash-salt:init
Hash salt already exists.
[error]  The "--id" option does not exist. 

I'm not sure where this --id is coming from or why it would be causing an issue with the changes from the patch.
Though I did verify that removing the changes from my local did allow me to deploy with a tag.

[danepowell]

I've pushed a fix to dev-main, could you please try again? Third time's a charm, let's hope :)

[shelane]

@danepowell It's working for me! I plan on doing more extensive tests in the morning.

[shelane]

@danepowell Tests this morning are mixed. My custom commands run fine as part of the cloud update hook. If I try to run my site install command, which is this:

  public function installSite() {
    // Note that a file permission exception is thrown on AC
    // so we have to catch that and proceed with the command.
    // @see: https://github.com/acquia/blt/issues/4054
    try {
      $commands = ['internal:drupal:install'];
      $strategy = $this->getConfigValue('cm.strategy');
      if (in_array($strategy, ['core-only', 'config-split', 'features'])) {
        $commands[] = 'drupal:config:import';
      }
// install default content module and site default content and uninstall site default content when complete
      $commands[] = 'custom:site:content';
      $commands[] = 'drupal:toggle:modules';
//read users in configuration to create users via drush after install
      $commands[] = 'custom:site:users';
      $this->invokeCommands($commands);
    }
    catch (BltException $e) {
      $this->say('<comment>Note:</comment> file permission error on Acquia Cloud can be safely ignored.');
    }
  }

from Acquia directly (ssh to ACE and use ./vendor/bin/blt for command calls in the appropriate environment)
./vendor/bin/blt custom:site:install --site=asc --environment=demo -n
Then I get this error:

[warning] Import strategy is config-split, but the config_split module does not exist. Falling back to core-only.

But config-split does exist and the command ran just fine in the hook. Why it is not recognizing it when I'm calling the command directly?

[danepowell]

This sounds like a result of #4314, I'll ping you in Slack but it might be worth opening a new issue.