search

ansible / proposals

Repository for sharing and tracking progress on enhancement proposals for Ansible.
Creative Commons Zero v1.0 Universal
93 stars 19 forks source link

Proposal: Load included role vars files from defaults folder. #21

Closed geerlingguy closed 5 years ago

geerlingguy commented 8 years ago

Currently, include_vars will include variables files from within a role's vars directory automatically. It would be beneficial to multi-platform or more complex roles to also search for include files within the defaults directory, so certain variables could be overridden by playbooks using the roles.

See PR for details.

pfink commented 8 years ago

+1

For me, the problem described by this proposal is one of the biggest obstacles to develop portable, configurable and redistributable roles.

bcoca commented 8 years ago

basically the same as this?

- name: Include OS-specific variables.
  include_vars: "{{item}}"
  with_first_found:
     - vars/{{ ansible_os_family }}.yml"
     - defaults/{{ ansible_os_family }}.yml"
geerlingguy commented 8 years ago

@bcoca - Yes... in that case, would included vars be able to be overridden like other defaults vars though?

bcoca commented 8 years ago

no, if you want that behavior it needs to be defined in defaults

pfink commented 8 years ago

@bcoca: I guess the use case is not fully clear to you? I recently posted a question to the mailing list describing the issue: https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/ansible-project/jFf1KfoWxBA/d_jm6QskAQAJ

At the moment, you have to decide between portability and configurability of a role, you cannot have both.

pfink commented 8 years ago

After a short walkthrough over the ansible core code, it seems like this is a rather invasive change. Maybe it would be easier to add a parameter that indicates if existing variables should be overwritten. This would also allow lot's of other use cases. Something like:

- include_vars: file=my_vars.yml overwrite=no

For this, I could submit a PR in a few days. Anyway, both approaches have some drawbacks: E.g., variables are not recognized in meta/main.yml. Also, if you use --start-at-task, it won't work.

Maybe we should start with the less invasive approach first and think of a more complete solution for the portability/configurability contradiction.

geerlingguy commented 8 years ago

@pfink - I think that could work (the overwrite=no option). And it would also be useful in certain cases outside of the use case of roles providing OS-specific included variables.

pfink commented 8 years ago

I referenced a commit showing the least invasive change to introduce this feature. Usage e.g.:

- include_vars: "defaults/{{ ansible_os_family | lower }}.yml"
  args:
    overwrite: no

(similar to the raw module)

Default for overwrite is, of course, yes.

While implementing and testing it, I realized that would just be some lines more to add <role>/defaults as a possible location for variables and to make the default value of overwrite dependent of the location where the variable file was found. So maybe this is not such a big change that a proposal is needed?

Anyway, if the defaults include mechanism would be implemented, it may wouldn't be the expected behaviour if e.g. a file like defaults/rhel.yml included with include_vars would not override variables defined in defaults/main.yml. To give higher precedence than the latter and lower than e.g. inventory variables seems to be much harder to implement.

What do you think?

kustodian commented 8 years ago

I would introduce a new module called include_defaults. The following task:

- include_defaults: Debian.yml

would load additional defaults from defaults/Debian.yml.

bcoca commented 8 years ago

I'm thinking this now might resolve most use cases:

- name: Include OS-specific variables.
  include_vars: "{{item}}"
  with_first_found:
    files:
     - {{ ansible_os_family }}.yml"
    paths:
     - vars
     - defaults
kustodian commented 8 years ago

It won't help, since include_vars overwrites variables which are defined in group_vars/host_vars. The point is to import OS specific defaults which won't overwrite group_vars/host_vars.

pfink commented 8 years ago

It's about writing reusable roles. If you can't overwrite role variables outside the role, they're not reusable and you have to import and modify them in every project.

tima commented 8 years ago

@geerlingguy @pfink I'm thinking include_vars is not the best place for this functionality to belong since where the file is sourced affects its precedence. Perhaps a key in the role's meta.yml file whose value could contain variables like {{ ansible_os_family }}.yml (main.yml being the default file name if not specified) would be a more elegant means of going about this.

bcoca commented 7 years ago

include_role now allows a 'defaults_from' parameter, is that sufficient to handle the use cases?

pfink commented 7 years ago

On the face of it, it looks like it does. I'll test it in our setup whether it has side effects to use include_role instead of the 'normal' role mechanisms.

A minor drawback of this approach in my opinion is that if I publish for a role on Github that has different default vars for different operating systems, I have to dictate the users of this role that they can't use the normal/recommended way to call it. They'll always have to call it via include_role with defaults_from: "{{ ansible_os_family }}.yml". I think it would be more elegant if I would be able to solve this concern inside the role.

kustodian commented 7 years ago

@bcoca if I use:

- name: Include OS specific defaults
  include_role:
    name: myrole
    defaults_from: {{ ansible_os_family }}.yml

would this task override variables which are set in group_vars/host_vars?

Also would be great if include_role by default used the current role if name parameter is not specified.

pfink commented 7 years ago

I added PR ansible/ansible#23738 now as I still think it makes it much more flexible if the user can decide whether he wants to overwrite existing variables with include_vars. Also, the implementation is very easy, so I see no reason of not offering the possibility.

pfink commented 6 years ago

fyi, this topic is currently also discussed at https://groups.google.com/forum/#!topic/ansible-devel/J22Sbo9p2cI

pfink commented 6 years ago

Here also some more major drawbacks for the defaults_from solution:

  1. When you do it like Brian suggested you have to maintain generic variables that are not dependent on the OS family, redundantly. This means, every default variable that is not OS dependent and would normally be put in the OS-independent defaults.yml, has to be copied to both files (windows.yml and linux.yml) and every time such a variable changes, you have to change it twice. That's very bad code style.
  2. It just allows to cover a single aspect, e.g. the OS family. Often, a role has to distingiush variables between many different aspects at the same time. So there would be e.g. different default variables for:

with defaults_from, you can, in the end, just load a single variable file, but to cover multiple aspects, you have load multiple variables files (e.g. linux.yml + ha.yml or windows.yml + standalone.yml, or ...)

lhoss commented 5 years ago

It won't help, since include_vars overwrites variables which are defined in group_vars/host_vars. The point is to import OS specific defaults which won't overwrite group_vars/host_vars.

+100 ( and no, it's not a solution to have to replace group_vars by --extra-vars files (-e @ ..))

For me (as a contributor to many roles, incl. complex multi OS supporting roles like https://github.com/hortonworks/ansible-hortonworks), this is the top issue with ansible roles/vars (AFAIK still no satisfactory solution or feature in latest ansible) ! So many hours wasted just tinkering about how to best work-around (depending on the role), and wondering is there really no better approach than the solution used p.eg also by @geerlingguy in his roles ( requiring 1 extra set_fact task (line) per variable (to override) incl. default vars with different name, p.eg. using the '__varname' ) 👎

I think there's quite a number of people that would really like a feature as proposed from @geerlingguy or similar (I'm adding links below), but maybe the urge for it is less obvious with the votes & discussions are too spread:

Related issues/PRs:

Related discussions:

lhoss commented 5 years ago

Being highly motivated to get progress on this topic I

The 2 most promising solution approaches IMHO:

Solutions in detail: (1) overwrite option to include_vars

(2) enhanced 'default vars' loading As @tima (comment above), I think that solution would be more elegant ...but probably more opinionated also. Pros/cons that i see:

As (for now) I think °1 should solve my issue mostly (unless I oversaw something), I'll ask @pfink if I can help fix the conflicts in his PR (and hopefully then we can have this merged to devel 👍 )

bcoca commented 5 years ago

we have voted against this several times, latest one is https://github.com/ansible/community/issues/474

so closing this proposal, feel free to resubmit if new arguments in favor appear.

lhoss commented 5 years ago

Yep, neither of above proposals are possible at the moment .. but we got some new ideas for an acceptable work-around (I will test out and show results here tomorrow)