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

User-configurable cron expression for regular cache flushing #2086 #2087

Conversation

LukasCesal
Copy link
Contributor

@LukasCesal LukasCesal commented May 16, 2022

Description (*)

(#2086)

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions github-actions bot added the Component: Core Relates to Mage_Core label May 16, 2022
Revert default cron expression to original 30 2 * * *
Copy link
Member

@elidrissidev elidrissidev left a comment

Choose a reason for hiding this comment

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

Tested and it looks good from my side.

<show_in_default>1</show_in_default>
<show_in_website>0</show_in_website>
<show_in_store>0</show_in_store>
<comment>Use 0 0 30 2 0 to never run cron, the recommended setting is 30 3 * * 6</comment>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why do you recommend to run weekly at 03:30 on Saturday instead of the current default of daily at 02:30?

Suggest to enclose the value with double quotes: "0 0 30 2 0", "30 3 * * 6".

Suggested change
<comment>Use 0 0 30 2 0 to never run cron, the recommended setting is 30 3 * * 6</comment>
<comment>Use "0 0 30 2 0" to never run cron, the recommended setting is "30 3 * * 6"</comment>

Copy link
Member

Choose a reason for hiding this comment

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

It's mentioned here: #2087 (comment)

@Sekiphp
Copy link
Contributor

Sekiphp commented May 17, 2022

For me is ok. Best option will be to merge this PR with original default value and create PR to 20.x for changed expression for weekly run.

@addison74
Copy link
Contributor

I analyzed OpenMage Backend and I found only one section where we can configure a cron job. Below is the implementation of the Magento team for Sitemap.

google_sitemap

This PR can be modified to provide the same timing and we would keep a standard format of the change in Backend. I would choose two values: Start Time with 3 drop-downs (hour, minute, second) and Frequency which can be a drop-down with the same options like in the image (Always, Hourly, Daily, Weekly, Monthly, Yearly, Never).

This is a more elegant and intuitive approach than filling in space delimited values in an input box with a help label bellow. In this way errors are avoided in entering wrong values that can affect the correct functionality.

@fballiano
Copy link
Contributor

@addison74 it's used somewhere like here:

Schermata 2022-05-19 alle 22 47 26

so to me it's really fine the way @LukasCesal built it :-)

@fballiano fballiano merged commit c789d30 into OpenMage:1.9.4.x May 19, 2022
@github-actions
Copy link
Contributor

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit c789d30. ± Comparison against base commit a640d7b.

Copy link
Contributor

@luigifab luigifab left a comment

Choose a reason for hiding this comment

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

A translate attribute is missing for cache tag: translate="label".
A translate attribute is missing for flush_cron_expr tag: translate="label comment".
Magento is now OpenMage, so instead of change that, I suggest to remove the word.
To never run cron, you just have to leave the field empty, right? I don't remember.

@Sekiphp Sekiphp deleted the User-configurable-cron-expression-for-regular-cache-flushing-#2086 branch May 21, 2022 21:11
@addison74 addison74 mentioned this pull request Jun 28, 2022
@addison74 addison74 mentioned this pull request Jul 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Core Relates to Mage_Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants