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

included include_role fails to escalate since ebf971f #35065

Closed
kiorky opened this issue Jan 18, 2018 · 17 comments
Closed

included include_role fails to escalate since ebf971f #35065

kiorky opened this issue Jan 18, 2018 · 17 comments
Assignees
Labels
affects_2.4 This issue/PR affects Ansible v2.4 bug This issue/PR relates to a bug. support:core This issue/PR relates to code supported by the Ansible Engineering Team.

Comments

@kiorky
Copy link
Contributor

kiorky commented Jan 18, 2018

ISSUE TYPE
  • Bug Report
COMPONENT NAME

include_role

ANSIBLE VERSION
2.4
devel
OS / ENVIRONMENT
  • ubuntu LTS
SUMMARY

Related to #33922 & #32565

@ansibot
Copy link
Contributor

ansibot commented Jan 18, 2018

@kiorky Greetings! Thanks for taking the time to open this issue. In order for the community to handle your issue effectively, we need a bit more information.

Here are the items we could not find in your description:

  • component name

Please set the description of this issue with this template:
https://raw.githubusercontent.com/ansible/ansible/devel/.github/ISSUE_TEMPLATE.md

click here for bot help

@ansibot ansibot added affects_2.4 This issue/PR affects Ansible v2.4 bug_report needs_info This issue requires further information. Please answer any outstanding questions. needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jan 18, 2018
@kiorky
Copy link
Contributor Author

kiorky commented Jan 18, 2018

cc @jimi-c

@ansibot ansibot removed needs_info This issue requires further information. Please answer any outstanding questions. needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly. labels Jan 18, 2018
@Akasurde Akasurde removed the needs_triage Needs a first human triage before being processed. label Jan 19, 2018
@sivel
Copy link
Member

sivel commented Jan 19, 2018

I wanted to take a moment, and update this issue with our plans for this issue.

We are going to work on a change for the 2.4.3 release, that will maintain the recursion fix, but revert the previous behavior to allow some of the attributes to include_* to merge down onto the included tasks.

Our plan for 2.5 currently, is to leave the functionality as is.

The reason for this decision and disparity between releases is due to the fact that it was never intended for include_* to merge down any attributes to the included tasks in 2.4.0, restricting that behavior only to import_*. The commit you referenced above actually fixed a bug in the code that allowed include_* to merge some attributes. However, since we try not to change behavior in a bug fix release like this change introduced, we'll revert the behavior change only for 2.4.

@sivel sivel changed the title included include_role fails to escalate since ebf971f931290a113f738f658d7a6e095994e120 included include_role fails to escalate since ebf971f Jan 19, 2018
@abadger abadger added this to Blocker in 2.4.x Blocker List Jan 19, 2018
@abadger abadger removed this from Blocker in 2.4.x Blocker List Jan 19, 2018
@kiorky
Copy link
Contributor Author

kiorky commented Jan 19, 2018

@sivel @abadger im not sure that will be the proper way to go as you then can't escalate a role that doesnt explictly "becomes" itself, like the test case i gave and is pretty inintuitive.

You can't then run a task somewhere that does:

- include_role: {name: foo}
  become: true

and expect foo to run as root.

Idem for the when statements, this defeats and breaks completly the initial use of include_role.

@kiorky
Copy link
Contributor Author

kiorky commented Jan 19, 2018

I'm also begging for a long time for your input on my #32565 PR, if someone can make things go a little bit further ;-)

@grassyknolling
Copy link

This looks related to #34974 "inconsistent documentation for tags on dynamic-include tasks"

@sivel where can we see which attributes will be merged down and which will not? Thanks!

@kiorky
Copy link
Contributor Author

kiorky commented Jan 19, 2018

in 2.5, None.

kiorky added a commit to corpusops/ansible that referenced this issue Jan 20, 2018
@kiorky
Copy link
Contributor Author

kiorky commented Jan 20, 2018

Really, the more i think to this, the more i think that for the special case of the become_ knobs, they should merge down to the executed tasks for include_tasks & include_roles.

This will be in other case disastrous in term of usability of include_* and DRY patterns.
As the only solution i see for the moment is to patch all include statements to add a vars: {somevar: true}

- include_role: {name: foo}
  vars: {somevar: true}

and also patch all the underlying, naive roles, that dont escalate themselves up to support escalating from the upper role by reacting on that somevar

# foo/tasks/main.yml
- become: "{{somevar}}"
  shell: foobar

This also dismiss the ability to reuse roles that is not under the OP control, as he wont be able to patch the naive role.

So, we seem there to be in an impasse.

kiorky added a commit to corpusops/ansible that referenced this issue Jan 21, 2018
@kiorky
Copy link
Contributor Author

kiorky commented Jan 25, 2018

@abadger @jimi-c:

  • The fix is working as intended for 2.4, thx !
  • It still fails on 2.5, it's OK for now as it is intended, but IMO an UX bug.
    Thx for the update, and really i'm awaiting of your input for that 'become' escalation topic.

kiorky added a commit to corpusops/ansible that referenced this issue Jan 30, 2018
kiorky added a commit to corpusops/ansible that referenced this issue Jan 31, 2018
kiorky added a commit to corpusops/ansible that referenced this issue Feb 8, 2018
@sivel
Copy link
Member

sivel commented Feb 13, 2018

We have restored behavior for 2.4, 2.5 will not inherit. Docs are in place for this.

I'm proceeding with closing this issue.

If you have further questions please stop by IRC or the mailing list:

@sivel sivel closed this as completed Feb 13, 2018
@kiorky
Copy link
Contributor Author

kiorky commented Feb 14, 2018

It is still a BIG UX bug (one of those than can make users like me stop using ansible)...

@kiorky
Copy link
Contributor Author

kiorky commented Feb 14, 2018

cc @abadger

@kiorky
Copy link
Contributor Author

kiorky commented Feb 14, 2018

We agreed that there is really a bug, as the third form in https://github.com/kiorky/ansible_test_cases/tree/35065b should already work to escalate a role from the caller

kiorky added a commit to corpusops/ansible that referenced this issue Mar 1, 2018
kiorky added a commit to corpusops/ansible that referenced this issue Mar 1, 2018
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bug_report labels Mar 7, 2018
kiorky added a commit to corpusops/ansible that referenced this issue Jun 6, 2018
kiorky added a commit to corpusops/ansible that referenced this issue Jun 6, 2018
kiorky added a commit to corpusops/ansible that referenced this issue Aug 1, 2018
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.4 This issue/PR affects Ansible v2.4 bug This issue/PR relates to a bug. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

No branches or pull requests

8 participants