-
Notifications
You must be signed in to change notification settings - Fork 583
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
Included handlers can be notified, but only by the included code #2316
base: devel
Are you sure you want to change the base?
Conversation
The old text can easily be (mis)read as: "Cannot trigger handlers (from) within includes".
Thanks for your Ansible docs contribution! We talk about Ansible documentation on Matrix at #docs:ansible.im if you ever want to join us and chat about the docs! We meet on Matrix every Tuesday. See the Ansible calendar for meeting details. We welcome additions to our weekly agenda items too. You can add the |
|
||
Notifying handlers Cannot trigger handlers within includes Can trigger individual imported handlers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kpinc The original phrasing seems correct to me. Can you provide examples of what behavior are you referring to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. In a role I have:
role/
tasks/
main.yaml
included.yaml
handlers/
main.yaml
main.yaml then includes included.yaml, which notifies a handler in handlers/main.yaml. And it works.
Look what I have done. I have literally "triggered a handler within an include". The included code caused a handler to execute. There is no other way to describe what happened than "triggering a handler within an include". But the text says I can't do this.
Point being, the text is vague and can be interpreted multiple ways. It means to say that you cannot trigger a handler that is declared within an included file. But that's not what it says. And, of course, you are allowed to trigger a handler from within an include. But it does not say that this is allowed.
The text needs refinement. My change takes of no more room than the original, at least as my browser renders. And I believe what I've written clarifies things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Do you think Cannot notify handlers defined within includes
or to be consistent with the statement about imports: Cannot notify individual included handlers
be better?
On Fri, 28 Feb 2025 00:42:49 -0800 Martin Krizek ***@***.***> wrote:
@mkrizek commented on this pull request.
>
- Notifying handlers Cannot trigger handlers within includes
Can trigger individual imported handlers
I see. Do you think `Cannot notify handlers defined within includes`
or to be consistent with the statement about imports: `Cannot notify
individual included handlers` be better?
I think "Cannot notify handlers defined within includes" is better.
Better than the original text and better than what I wrote in
the patch, which is slightly 'round-the-barn.
Because you raised the issue...
I don't like the "import sentence". "Individual" is irrelevant,
an extra word that adds nothing. Better than "Can trigger
individual imported handlers" would be "Can notify handlers
defined within imports". ("Trigger" is all well and good,
but the vocabulary is "notify".)
Thinking about it, I prefer "declare" to "define".
(The docs don't seem to use much of either word,
although variables are "defined" in a single sentence
I found.)
And, may as well be explicit about what we're talking
about -- not the include or import statement but
the code included or imported.
So:
Cannot notify handlers declared within included code
and
Can notify handlers declared within imported code
How does this process go from here? Should I add
another patch to make this change and push it?
Is there other phrasing you prefer?
Thanks for the help reviewing.
Regards,
Karl ***@***.***>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
|
On Fri, 28 Feb 2025 08:18:15 -0600
"Karl O. Pinc" ***@***.***> wrote:
On Fri, 28 Feb 2025 00:42:49 -0800
Martin Krizek ***@***.***> wrote:
> @mkrizek commented on this pull request.
>
>
>
> >
> - Notifying handlers Cannot trigger handlers within
> includes Can trigger individual imported handlers
>
> I see. Do you think `Cannot notify handlers defined within includes`
> or to be consistent with the statement about imports: `Cannot notify
> individual included handlers` be better?
I think "Cannot notify handlers defined within includes" is better.
So:
Cannot notify handlers declared within included code
Except, this is wrong. You can do, say, an import-playbook
and that can contain handlers which the imported code can
notify.
So:
Cannot notify handlers declared within included code,
unless the included code is notifying
(Which is what my patch says, but the above is more clear
and uses more words.)
Regards,
Karl ***@***.***>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
|
On Fri, 28 Feb 2025 08:30:55 -0600
"Karl O. Pinc" ***@***.***> wrote:
On Fri, 28 Feb 2025 08:18:15 -0600
"Karl O. Pinc" ***@***.***> wrote:
> On Fri, 28 Feb 2025 00:42:49 -0800
> Martin Krizek ***@***.***> wrote:
>
> > @mkrizek commented on this pull request.
> >
> >
> >
> > >
> > - Notifying handlers Cannot trigger handlers within
> > includes Can trigger individual imported handlers
> >
> > I see. Do you think `Cannot notify handlers defined within
> > includes` or to be consistent with the statement about imports:
> > `Cannot notify individual included handlers` be better?
>
> I think "Cannot notify handlers defined within includes" is better.
>
> So:
>
> Cannot notify handlers declared within included code
Except, this is wrong. You can do, say, an import-playbook
and that can contain handlers which the imported code can
notify.
So:
Cannot notify handlers declared within included code,
unless the included code is notifying
"Cannot notify handlers declared within included code,
unless such notification is done within the included code"
The above is overly long, but clear. It makes sense to me
to put everything after the comma in a footnote. (The exception
to the rule needs to be documented somewhere.) Using a
footnote make it suitable for "table content".
The text of the patch attempts to express the rule and
the exception, but, from a practical perspective, is too
terse. Thoughts?
Regards,
Karl ***@***.***>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
|
"define" is what is used in
I am not sure "included code" is established terminology.
I think we need to step back here. The existing text:
refers to the situation where |
On Mon, 03 Mar 2025 01:45:16 -0800 Martin Krizek ***@***.***> wrote:
mkrizek left a comment (ansible/ansible-documentation#2316)
> Thinking about it, I prefer "declare" to "define".
"define" is what is used in
`docs/docsite/rst/playbook_guide/playbooks_handlers.rst`.
Ok.
> ... You can do, say, an import-playbook
`import_playbook` cannot be used as a handler.
---
- name: Update web servers
hosts: webservers
remote_user: root
tasks:
- name: Ensure apache is at the latest version
ansible.builtin.yum:
name: httpd
state: latest
- name: Write the apache config file
ansible.builtin.template:
src: /srv/httpd.j2
dest: /etc/httpd.conf
notify: reload apache
handlers:
- name: Keep apache updated on config change
ansible.builtin.systemd_service:
name: httpd
state: reloaded
listen: reload apache
Do an include_playbook on the above playbook and
what can happen? You have notified a handler defined
in an include, just what the current documentation
says you can't do.
> ... included code ...
I am not sure "included code" is established terminology.
> "Cannot notify handlers declared within included code,
unless such notification is done within the included code"
There needs to be terminology for the two sides of the
handler notification. The code that declares the handler
and the code that notifies the handler. The problem
with what's written now is that it considers only one
side of the equation.
If "included code" isn't a meaningful phrase there's always
"code that is included".
I think we need to step back here. The existing text:
```
Cannot trigger handlers within includes Can trigger individual
imported handlers ```
refers to the situation where `include_tasks` or `import_tasks` is
being used as a handler.
That's what it is referring to, but that is not what is says it is
referring to. It is making a unconditional statement. Which is
a problem because it is in a table that's supposed to be a quick
reference. Someone should be able to read just the table, and not
any surrounding text, and have a full understanding of each table
cell.
The behavior is already described in
https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_handlers.html#includes-and-imports-in-handlers.
So unless we are able to described it in short we might want to just
link to that section of the handlers documentation.
It does not seem that hard to come up with a sentence.
"Cannot notify handlers defined within the code which is included,
unless such notification is done within the code which
is included"
The above works for me to completely describe the
situation. To get something more "in short", you have to
go back to my original patch. And that is clearly
too succinct to be meaningful to most people.
Otherwise, writing:
"Usually cannot notify handlers declared within included code"
and then linking to someplace that fully explains the issues
would work. But seems somewhat useless, at least as I write
this, because it kind of defeats the point of having the table.
You may as well just write: "See here for an explanation".
Regards,
Karl ***@***.***>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
|
The old text can easily be (mis)read as: "Cannot trigger handlers (from) within includes".