Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

M2 recipe : handle cron stop in try catch #3828

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gbobts
Copy link
Contributor

@gbobts gbobts commented May 10, 2024

This change handle the case when cron:remove can not be executed properly, for example when setup:upgrade or Magento 2 installation wasn't done properly.

Try/Catch prevents breaking of deployment (as cron stopping is nice to have, but not required).

@peterjaap
Copy link
Contributor

I'm not entirely sure this should be in the default recipe at all. Installing the cron is a one-off task and shouldn't occur on every deployment.

@antonmedv antonmedv closed this May 17, 2024
@gbobts
Copy link
Contributor Author

gbobts commented May 17, 2024

  1. I believe stopping cron jobs while deploying is a good practice
  2. The path to bin/magento in the crontab will contain the release number while executing cron:install manually, so this path should be updated by the deployment process

So I believe this MR should be accepted

@antonmedv antonmedv reopened this May 17, 2024
}
}

run('pgrep -U "$(id -u)" -f "bin/magento +(cron:run|queue:consumers:start)" | xargs -r kill');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this approach. What if cron job does not handle SIGTERM?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand, however I'm not sure to be able to improve this code (that is already merged in 7.4.0)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants