role|name of task bug with includes

I re-post beneath the exchange with James Cammarata on ansible-project.

James, your patch

if os.path.isfile(handler):

  • nt = dict(include=pipes.quote(handler), vars=role_vars, role_name=role[‘role’])
  • nt = dict(include=pipes.quote(handler), vars=role_vars)

is about the “notify: role|name of task” bug issue, you removed the prepending of role| in front of the handlers.

I thought it was on purpose that role| was added in front of handlers. Namespacing the handlers by role makes a lot of sense to me.

don’t you think it was acceptable as it was ? not a bug but a feature ?

Task names should NOT be prepended either, just the output in the callbacks.

– Michael

That is a short-term fix so that people running from devel would not have their playbooks broken. I will get this fixed for handlers as well, however it will involve adding an additional field to the task structure to save the role name separately from the task name, and then having the output functions use that new field as well. That will take a bit more testing, so I wanted to get people back up and running that might be having issues with notifications. I definitely want the handlers to show the role namespace as well.

The above will also address what Michael was talking about.

Thanks!

We do not want any backwards incompatible playbook changes where someone can’t notify a handler defined in another role.

– Michael

w00t

– Michael

Yes that’s a mighty goal. I understand this is important for the project.

I just thought that adding a level of role namespacing in the name of tasks was a super feature because I already sometimes have conflicting names like ‘restart service’. If a way can be found using the presence of ‘|’ to make this backwards compatible that’s great !

James thank you for commiting temp-fixes on devel I like being on the edge but playbooks that break are disappointing, even on devel :wink:

regarding the role prefixing, I imagined the

role>name of task becoming

my_role_dir/role|name of task
or
http://ansible.mydomain.com/roles/nginx | name of task

for such extensions & shareability of roles, the namespacing would be necessary to avoid naming conflicts I suppose.

No to prefixing, need to assign service specific names

– Michael

Yeah that’s a lot of text on the line potentially, so best to keep your roles prefixed yourself to a degree. For instance, if you’re using a role from user foo you got from github, you could easily just name the role directory github.foo.rolename to namespace it yourself.

but from what I understand handler’s names are “global”

so you mean that people have to give a unique name to all their handlers across all their roles and all the roles of other people they will share roles with ?

I am not sure what happens a playbook discovers a handler with a name that a previous handler uses. Does it fail saying : you should rename your handler, this name is already taken ?

It is currently hash merge related so it is NOT a problem if two roles define restart nginx

I consider this desirable and actually making sharing easier as they are likely to have the same definition anyway.

– Michael

I had a bug today :

a notify: restart nginx was apparently “restarting” nginx.
the ansible output was “changed” and not “failed”
but the server was not restarted.

after closer inquiry (and don’t ask why :wink: I had 2 handlers

  • name: restart nginx
    service: name=nginx state=restarted

and

  • name: restart nginx
    sudo_user: root
    service: name=nginx state=restarted

the 2 were doing a “changed” but only the one with sudo_user:root worked !

I have no explanation as to why it was/was not working but I was puzzled for some time.

If this kind of collision exists via shared roles it can become a nightmare I think.

Most people set sudo on the play. Not too worried.

– Michael

The above callback method has been merged in, if you’d like to test it.

Hello,
I just tested.

  • (1) the notification is working ok, notified task has the r|
  • (2) there is still a bug regarding the name of included tasks

in the (2) I a in the case where a role/r/tasks/main.yml has an include: install.yml directive

all the tasks in the install.yml file do not have the r| prepended in their name in the output.

do you confirm this is a bug ?

Hi Jerome,

Thanks for the heads up, I would agree that includes should also prefix the name of the role.

Please make sure there is bug in github so we have a record of this.

done
https://github.com/ansible/ansible/issues/4223