Add another check for setup:upgrade

[lamasfoker]

I have added another check if the setup:upgrade command is needed to be run.
As stated on the command class module:config:status if it fails you have to run the setup:upgrade command.

[lamasfoker]

@peterjaap could you please take a look at this?

[gbobts]

@peterjaap @lamasfoker is this still required ?

[lamasfoker]

This exception is thrown also on the latest magento 2 version, so yes.

[gbobts]

@lamasfoker thanks for your contribution ! Can you update the doc through a new commit ? Looks like that's blocking the pull request checks

[lamasfoker]

Done

[norgeindian]

Since this change is included, we're facing issues with modules, which are only a dev requirement in the composer.json file.
I debugged that in \Magento\Setup\Console\Command\ModuleConfigStatusCommand::execute and found out, that these modules are somehow included in the $currentModuleConfig, but not in $correctModuleConfig.
This way, setup:upgrade is run in every deployment, which leads to unneeded downtimes.
Does someone see a good way of fixing that?

[lamasfoker]

@norgeindian I do not have this issue. Do you have any examples of magento modules required only on dev? Perhaps you are running composer install without --no-dev parameter on your CI/CD?

[norgeindian]

@lamasfoker, we have a few modules, which we only use locally.
So for example https://github.com/ho-nl/magento2-Ho_Templatehints.
This is definitely something you only want locally and don't need on a live server.
That means, that we have an entry in the config.php like this:
'Ho_Templatehints' => 1

As soon as we deploy, Magento sees, that this module is in the config.php, but it's not used in the code.
In earlier times, we used https://github.com/jalogut/magento2-deployer-plus/blob/a8c4432a68a4ac442541f250577e6049abaed52b/recipe/magento_2_2/config.php#L43, where the modules were removed one by one, but until now, this was not needed anymore.
How do you maintain your config.php?

To reproduce my issue, please include the module I mentioned above in your require-dev section of your composer.json, run setup:upgrade to make sure, that it is locally added to the config.php file and try to deploy.
Then you should face the same issue as we do. The check always returns the information, that maintenance should be enabled and setup:upgrade should be run.

I already tried your --no-dev approach, but that did not help, as the problematic point is the config.php. The modules are correctly not installed, like I expect it to be.

[norgeindian]

@lamasfoker , were you able to reproduce this issue on your end the way I wrote or is it actually something else, which is related to our project setup?

[lamasfoker]

Sorry for the late replay.

How do you maintain your config.php?

We add app/etc/config.php as shared_files and we do not include it in GIT. I pasted below our recipe from one of our magento 2 installation. The deployer version is 7.3.0 so without this PR that was released with 7.4.0

<?php

namespace Deployer;

use Deployer\Exception\RunException;

const CONFIG_PHP_UPDATE_NEEDED_EXIT_CODE = 1;

require 'recipe/magento2.php';

set('clear_paths', [
    '{{magento_dir}}/pub/static/_cache/*',
    '{{magento_dir}}/var/generation/*',
    '{{magento_dir}}/var/cache/*',
    '{{magento_dir}}/var/page_cache/*',
    '{{magento_dir}}/var/view_preprocessed/*'
]);

set('keep_releases', 5);
add('shared_files', ['pub/.htaccess', 'pub/.htusers', 'pub/robots.txt', 'app/etc/env.php', 'var/.maintenance.ip', '.env.local', 'app/etc/config.php']);
add('shared_dirs', ['bash/bin/logs', 'pub/lamasfoker-downloads']);
set('artifact_file', 'artifact.tar.gz');
set('artifact_dir', '.');
set('cachetool_url', 'https://github.com/gordalina/cachetool/releases/download/9.0.0/cachetool.phar');

desc('Remove jobs of bin/magento queue:consumers:start');
task('magento:remove-append-consumers', function () {
    run("ps -ef | grep 'bin/magento queue:consumers:start' | grep -v grep | awk '{print $2}' | xargs -r kill -9");
});

desc('Kill running magento cron job');
task('magento:kill-running-cron-job', function () {
    run("ps -ef | grep 'bin/magento cron:run' | grep -v grep | awk '{print $2}' | xargs -r kill -9");
    run("ps -ef | grep 'bin/magento lamasfoker:erp:customer:sync --all' | grep -v grep | awk '{print $2}' | xargs -r kill -9");
});

desc('Switch magento cron');
task('magento:switch-cron', function () {
    run('{{release_path}}/cronjob_switcher.sh');
});

set('database_upgrade_needed', function () {
    // detect if setup:upgrade is needed
    try {
        run('{{bin/php}} {{bin/magento}} setup:db:status');
    } catch (RunException $e) {
        if ($e->getExitCode() == DB_UPDATE_NEEDED_EXIT_CODE) {
            return true;
        }

        throw $e;
    }
    try {
        run('{{bin/php}} {{bin/magento}} module:config:status');
    } catch (RunException $e) {
        if ($e->getExitCode() == CONFIG_PHP_UPDATE_NEEDED_EXIT_CODE) {
            return true;
        }

        throw $e;
    }

    return false;
});

desc('Deploy your project');
task('deploy', [
    'deploy:info',
    'deploy:setup',
    'deploy:lock',
    'deploy:release',
    'artifact:upload',
    'artifact:extract',
    'deploy:shared',
    'deploy:clear_paths',
    'magento:switch-cron',
    'magento:kill-running-cron-job',
    'magento:maintenance:enable-if-needed',
    'magento:upgrade:db',
    'magento:maintenance:disable',
    'deploy:symlink',
    'magento:switch-cron',
    'magento:remove-append-consumers',
    'magento:cache:flush',
    'cachetool:clear:opcache',
    'deploy:cleanup',
    'deploy:unlock',
    'deploy:success',
]);

after('deploy:failed', 'deploy:unlock');

[norgeindian]

@lamasfoker , thanks for the explanation. That would of course solve it.
But we actually use the config.php to fix config settings and this way we would like to keep it in git.
What do you think of adding a check, if the config.php is part of the shared files and if it is, the check should be run and if not, we skip it?

[lamasfoker]

Sorry, but I think your proposal is a workaround and not a solution. I am not sure about a proper solution (and so a PR to deployer), but you can add to your recipe the config:remove-dev-modules task. Let me know what you think.

[sprankhub]

@lamasfoker, from https://experienceleague.adobe.com/en/docs/commerce-operations/configuration-guide/deployment/overview:

The shared configuration file, app/etc/config.php, should be included in source control so it can be shared between development, build, and production systems.

Hence, I think running the check module:config:status during deployment is wrong. Your CI checks should make sure you only commit / merge valid code, so that your app/etc/config.php should be up-to-date when you deploy. During deployment, it shouldn't be changed, IMHO. That is why I also think the check should be removed again.

@peterjaap, may I ask for your opinion here?

[lamasfoker]

@sprankhub the docu said app/etc/config.php is a shared configuration file, so what's written inside is true for all the environments: development, build, and production. If it's not true, then it should be possible to override it per environment, but as you can see here is not possible to override module status with app/etc/env.php. In addition take a look to this magento issue, it's not clear why config.php has to be included in GIT. I think that's a Magento limitation. So we decided to not include it.

Your CI checks should make sure you only commit / merge valid code, so that your app/etc/config.php should be up-to-date when you deploy.

@norgeindian said:

As soon as we deploy, Magento sees, that this module is in the config.php, but it's not used in the code.

In your case I think config.php is not up-to-date; that's why I suggest disabling it during deployment.

[peterjaap]

@sprankhub @lamasfoker the reason why we check for it is because it is possible to add a Magento extension through Composer, and then forgetting to run setup:upgrade locally. The pipeline then runs setup:upgrade, effectively enabling the extension. Most of the times this is what you want, but sometimes this is unexpected behavior. We therefore run the module:config:status check to make sure the extension that is added is explicitly enabled.

[sprankhub]

Thanks for your feedback, @peterjaap. "forgetting to run setup:upgrade" is a case, which should / could be handled in CI, but I don't think this should be done during the deployment. This would mean that during deployment, a module is suddenly enabled, which was previously not. Nothing a deployment process should do, IMHO.
Also, how do you handle Magento modules, which are in require-dev? If you have such, module:config:status will always report that a DB upgrade is needed, so that you never have zero downtime deployments any more.

[peterjaap]

@sprankhub true, we have it in CI, and have it disabled in this specific recipe. But it's here for those that don't run CI but deploy from their local terminal (which I've found a lot of people who use this recipe do).

We don't deploy modules in require-dev, since we run composer install --no-dev for production. So those modules aren't included (but are listed in config.php but that's not an issue).

[sprankhub]

We don't deploy modules in require-dev, since we run composer install --no-dev for production. So those modules aren't included (but are listed in config.php but that's not an issue).

For production, we of course also use --no-dev. I guess your config.php is also in git? Then it will also include the dev modules, right? When you then deploy with --no-dev, the module:config:status will see the dev modules in config.php (but not in the vendor directory) and will complain that you need to run setup:upgrade, leading to a deployment with downtime.

[sprankhub]

@peterjaap, could you have another look here? I truly believe this PR literally broke zero downtime deployment for many people.