feat(hourly): Add configuration to enable hourly jobs #60
Conversation
mdschmitt
force-pushed
the
add_hourly_capability
branch
from
6194ab5 to
4ba20c8
Compare
There was a problem hiding this comment.
Thanks for the PR, @mdschmitt. Some initial feedback for the time being.
After numerous discussions, essentially we never use salt['pillar.get']() in our formulas. Let me give some brief explanations:
salt['pillar.get']()is unnecessary sincemap.jinjaalready merges all of the elements under one key, which islogrotatefor this formula.- There have been many reasons given why non-secret data shouldn't be stored under pillars (e.g. pillars are rendered on the master); so we're in the process of moving to
config.getinstead, which has already been adopted by this formula inmap.jinja.
Hence, using salt['pillar.get']() inline is unnecessary duplication, longer to write, more difficult to review (not a single point for all data being used by the formula), not to mention prevents end-users from providing their data from sources other than pillar (e.g. SDB).
You should find it relatively simple to access the data via the logrotate key. Let me know if you face any issues. Once you're done, please amend/rebase your commit(s) and force-push back here -- thanks.
mdschmitt
force-pushed
the
add_hourly_capability
branch
from
4ba20c8 to
7b5676b
Compare
|
@myii , how's this look? |
|
No major rush, but any word on this? |
|
@myii , dropping another bump in here. Any feedback on my revisions? |
mdschmitt
force-pushed
the
add_hourly_capability
branch
2 times, most recently
from
c274d01 to
77374a4
Compare
mdschmitt
force-pushed
the
add_hourly_capability
branch
from
77374a4 to
cd4cd1d
Compare
|
Latest CI failure because ArchLinux properly uses systemd timers rather than cron. Requires deeper code tweakage to properly provide for. This PR on hold until I can get that to be better. |
|
@mdschmitt Thanks for the additions. I've tested some changes in my own fork to ensure Arch Linux is working as well. I'll push those here and merge this PR shortly. One question remains: which reference did you use for the |
Use to fix Arch Linux `cron` installation and testing in the formula: * saltstack-formulas/logrotate-formula#60 (comment)
|
🎉 This PR is included in version 0.13.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
# [1.347.0](v1.346.0...v1.347.0) (2022-01-14) ### Features * **logrotate:** use `cron-formula` dep instead of single `cron` state ([bb0ec87](bb0ec87)), closes [/github.com/saltstack-formulas/logrotate-formula/pull/60#issuecomment-1013031092](https://github.com//github.com/saltstack-formulas/logrotate-formula/pull/60/issues/issuecomment-1013031092)
|
Wow @myii , thank you so much for doing this. You beat me to writing the kitchen test and I definitely would have ended up rewriting things to allow for Arch. Folding in As far as the naming for "logrotate.hourly.d", it was just a logical canonicalization of "logrotate.d" with the explicit addition of "hourly" since hourly isn't out-of-the-box functionality. Inspiration taken from https://sleeplessbeastie.eu/2018/07/11/how-to-execute-logrotate-every-hour |
|
No problem -- thanks for the reference, @mdschmitt. |
PR progress checklist (to be filled in by reviewers)
What type of PR is this?
Primary type
[build]Changes related to the build system[chore]Changes to the build process or auxiliary tools and libraries such as documentation generation[ci]Changes to the continuous integration configuration[feat]A new feature[fix]A bug fix[perf]A code change that improves performance[refactor]A code change that neither fixes a bug nor adds a feature[revert]A change used to revert a previous commit[style]Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)Secondary type
[docs]Documentation changes[test]Adding missing or correcting existing testsDoes this PR introduce a
BREAKING CHANGE?Not unless existing pillar values for clients have "hourly" set in their job configurations. ...in which case, they already don't work straight-out-of-the-box.
Related issues and/or pull requests
Describe the changes you're proposing
Pillar / config required to test the proposed changes
Debug log showing how the proposed changes work
Documentation checklist
README(e.g.Available states).pillar.example.Testing checklist
state_top).Additional context