Repository navigation
Use the systemd module to reload systemd instead of ansible.builtin.command - #215
Open
gohri81-tech wants to merge 1 commit into
Open
gohri81-tech wants to merge 1 commit into
gohri81-tech wants to merge 1 commit into
Conversation
The handler shelled out to systemctl daemon-reload through ansible.builtin.command, which always reports changed and cannot run in check mode. ansible.builtin.systemd with daemon_reload does the same work as a module, and the module is already used by the restart handler right below it. The two handlers are left separate because tasks notify them independently. Closes opensearch-project#207 Signed-off-by: Gaurav K Ohri <gohri81@gmail.com>
gohri81-tech
requested review from
TheAlgo,
gaiksaya,
peterzhuamazon,
prudhvigodithi and
saravanan30erd
as code owners
September 17, 2026 16:28
Author
|
A gentle reminder on this one whenever someone has time. It replaces the Happy to adjust it if you would prefer a different shape. Thank you. |
Author
|
Gentle nudge - no review since this was opened on 17 September. It swaps the ansible.builtin.command call for the systemd module's own daemon_reload, so the handler becomes idempotent and works under --check (closes the linked issue). The one red check is the merit-badger action, not the change itself. Could a code owner take a look? Thank you. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes #207.
The
reload systemd configurationhandler inroles/linux/opensearch/handlers/main.ymlshelled out tosystemctl daemon-reloadwithansible.builtin.command. Using the module instead means the task no longer reportschangedon every run, works under--check, and no longer needs acommand/shellexception from ansible-lint.ansible.builtin.systemdis already used by therestart opensearchhandler directly below, so this keeps the file consistent with itself rather than introducing a new module.The issue suggested folding the reload into a single
restart opensearchhandler withdaemon_reload: yes. I kept the two handlers separate on purpose, because tasks notifyreload systemd configurationon its own and merging them would restart the service in cases where only a unit file changed.I used
ansible.builtin.systemdrather than the neweransible.builtin.systemd_servicename so that the role keeps working on ansible-core versions older than 2.16. Happy to switch if you would rather move to the new name.Issues Resolved
Closes #207
Check List
--signoffI don't have an environment here to run the playbook against a real host, so please treat CI and your own run as the source of truth.