DT-587: Changes to checkDirty behavior during deployments

[danepowell]

BLT has long had a function checkDirty that fails deploys if BLT detects that files have changed during the deploy process itself, which could indicate a variety of problems such as bad Composer files, NPM files, problems with gitignore, and/or more generally conditions that might cause the site to behave differently in a remote environment than it did during tests (with undefined and potentially very bad consequences).

In 9.2.x and 10.x we recently enabled this feature on Pipelines (it's always been enabled for Travis and other environments that directly call blt deploy. Pipelines is a special case because it doesn't call deploy).

It turns out a lot of Pipelines users have those kinds of problems with their codebase, as evidenced by the spate of issues opened after this feature was added. While it's good that these problems are being caught and corrected, it's not great that it started happening unexpectedly, or that it happens during the deploy step (it's really bad for things to fail during a deploy if they didn't fail during tests).

I propose the following changes:

• Add a more helpful error message to both 9.2.x and 10.x branches indicating corrective action if checkDirty fails.
• See if there's some way to run or simulate this check earlier than the deploy step, and ideally fail during tests.

This last point is tricky because a lot of things (such as the gitignore file, and composer production dependencies) are different during the deploy process, so we can't just run the same command earlier on. I see two options:

• Based on feedback from users who have had problems with their codebase, see if there are additional specific validation steps that we can perform, such as checking for whether settings files are valid.
• Maybe we could simulate a deploy as part of the test process, by running a deploy with the dry-run option? We'd need to make this work on Pipelines as well as Travis (not easy, since they have very different deploy processes), and only in cases when a deploy would otherwise be called for.

[danepowell]

If we generate documentation around this, point out the workaround mentioned for NPM users in #3526 (comment) (use npm ci).

[danepowell]

#3572 is a documentation fix. The rest of this I think will need to wait until a later release.

[justinlevi]

@danepowell Should this documentation also be added to the 9.2.x branch?

Also not sure the documentation clearly addresses the error as it arrises in the default acquia-pipelines.yml provided by BLT via $ blt recipes:ci:pipelines:init. Could very well be my misunderstanding though...

Using npm install can sometimes cause package-lock.json to change during a deploy. Using npm ci instead should avoid that.
Try replicating the CI process locally by running the same commands (visible in the CI logs), such as blt setup and blt tests:all. If these change files locally, you should determine if these changes need to be committed or whether your test scripts need to be adjusted to avoid creating changes.
Run blt doctor locally to ensure that there are no problems such as missing settings file includes.
See this issue for additional documentation and solutions.

• npm install is not obviously being called in the pipelines file so fixing it with npm ci is problematic
• blt tests:all passes without issue but the error persists on pipelines
• blt doctor presents no issues
• the link to DT-587: Changes to checkDirty behavior during deployments #3564 doesn't help either as it just leads you back to the PR where the documentation came from.

To further clarify, the issue/problem as I see it, is that source:build:frontend is called as part of the setup-app step provided by default in the acquia-pipelines.yml "out-of-the-box" via the$ blt recipes:ci:pipelines:init

      - setup-app:
          type: script
          script:
            - source ${BLT_DIR}/scripts/pipelines/setup_app

The setup_app script lives in the vendor folder and calls blt setup --define drush.alias='${drush.aliases.ci}' --environment=ci --no-interaction --ansi --verbose

which in turn calls source:build:frontend here in `src/Robo/Commands/Setup/BuildCommand as the last step:
https://github.com/acquia/blt/blob/10.x/src/Robo/Commands/Setup/BuildCommand.php#L91

  public function build() {
    $this->invokeCommands([
      'tests:behat:init:config',
      // source:build:composer must run prior to blt:init:settings to ensure
      // that scaffold files are present.
      'source:build:composer',
      'blt:init:git-hooks',
      'blt:init:settings',
      'source:build:frontend',
    ]);

    $this->invokeHook("post-setup-build");
  }

build:frontend then calls npm install or npm build as per detailed in the Frontend documentation here:

https://github.com/acquia/blt/blob/061a0f5c7fa191020a89d0362b8fc3cf0d1b329f/docs/frontend.md#build

Perhaps the FrontEnd documentation needs to be modified to also detail this issue as a "gotcha".

[danepowell]

I don't think backporting the docs would be appropriate in this case since 9.2.x is only receiving security updates as of tomorrow.

When I mentioned npm install, it's only as one example in a list of many potential culprits. Obviously not all will be relevant to every project. BLT never directly calls npm install, but individual projects quite frequently run it as part of their frontend build hooks. I can try to document that more explicitly.

[danepowell]

By the way, your sequence is a little off. While it's true that source:build:frontend is called as part of blt setup during CI runs, that's not why the assets get deployed. source:build:frontend is actually invoked directly by the deploy command subsequent to the initial setup:

blt/src/Robo/Commands/Deploy/DeployCommand.php

Line 337 in 27e9f00

'source:build:frontend',

Functionally I don't think it's that different, but just so you're aware...

[wintercreative]

Thanks for the clarification on my sequence. I think you're right though, functionally it's still the same problem. I wonder if the frontend documentation would benefit from adding some information on this and how to solve it given lots of projects run npm install in a frontend build hook.

[danepowell]

I added some documentation regarding this yesterday: #3602

[andreas-hs]

Hello @danepowell !
Thank you for your nice job 👍
I added a new pull request which can be related to this issue #3885
The reason I made it was one bug.
We spent a lot of time understanding what breaks the build.
It was a changes during a drupal installation.
There were similar situations, some times ago, when permissions to the settings.php file were changed and it was difficult to immediately understand why the build was breaking.

How do you look at adding the ability to see what exactly has changed in the build process?
This can save a lot of time. In this case, in the console we will see something similar to this instead this one.
What do you think?

[adamfchs]

How does one actually implement the --ignore-dirty flag on the build process kicked off when a commit is pushed? I get an error telling me to implement that flag but with no instructions on how to do so:

[error]  There are uncommitted changes on the source repository (listed above). Commit, stash, or remove these changes before deploying, or use the --ignore-dirty flag. Additional guidance is available at https://support.acquia.com/hc/en-us/articles/360035204013-Dirty-BLT-source-directory-prevents-deploys.
For troubleshooting guidance and support, see https://docs.acquia.com/blt/support/ 

I did follow the link in the error and tried the 'npm ci' instead of 'npm install' solution there but that fails for a different reason:
npm WARN Local package.json exists, but node_modules missing, did you mean to install?

My frontend-reqs and frontend-assets commands work great locally but pipelines has been failing the last 5 hours as I try everything I can think of and/or find online.

[ejanus]

I've created the following issue in my attempt to add --ignore-dirty and how to properly implement the travis deploy scripts. Hoping to hear back soon as I really need to get BLT updated and pushed upstream. #4111