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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

geerlingguy
Copy link

@geerlingguy geerlingguy commented Jul 10, 2016

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
Copy link

pfink commented Jul 14, 2016

+1

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

@bcoca
Copy link
Member

bcoca commented Jul 15, 2016

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
Copy link
Author

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

@bcoca
Copy link
Member

bcoca commented Jul 15, 2016

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

@pfink
Copy link

pfink commented Jul 15, 2016

@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
Copy link

pfink commented Jul 20, 2016

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
Copy link
Author

@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
Copy link

pfink commented Jul 20, 2016

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
Copy link

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
Copy link
Member

bcoca commented Aug 18, 2016

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
Copy link

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
Copy link

pfink commented Aug 20, 2016

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
Copy link

tima commented Aug 20, 2016

@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.

pfink added a commit to atosorigin/ansible that referenced this pull request Sep 8, 2016
# Conflicts:
#	lib/ansible/plugins/action/include_vars.py
@bcoca
Copy link
Member

bcoca commented Feb 3, 2017

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

@pfink
Copy link

pfink commented Feb 4, 2017

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
Copy link

@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
Copy link

pfink commented Apr 19, 2017

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
Copy link

pfink commented Apr 13, 2018

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

@pfink
Copy link

pfink commented Jul 27, 2018

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:
  • OS family (linux.yml, windows.yml)
  • high-availibility / standalone setup (ha.yml, standalone.yml)
  • different network configurations
  • ...

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
Copy link

lhoss commented Mar 6, 2019

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:

Values populated by the "include_vars" of "vars/tune/*.yml" override pretty much everything... but I want values in host_vars to take precedence over these.

JM1 added a commit to JM1/ansible-role-jm1-container-docker that referenced this pull request May 8, 2019
Role-specific default values for variables are now only used if these
variables have not been assigned earlier. This requires a workaround
using set_fact because Ansible does not provide a include_defaults like
function yet and role variables have a high precedence. For discussion
and solutions, see: ansible/proposals#21

Required packages are now installed using virtual packages
jm1-container-docker via role virtual_pkg. This makes it easier for
users to remove ansible-installed packages later.
JM1 added a commit to JM1/ansible-role-jm1-dev-cpp that referenced this pull request May 8, 2019
Following the split of role 'core' we now depend on role base_system.

Required packages are now installed with virtual packages via role
virtual_pkg. This makes it easier for users to remove ansible-installed
packages later.

Role-specific default values for variables are now only used if these
variables have not been assigned earlier. This requires a workaround
using set_fact because Ansible does not provide a include_defaults like
function yet and role variables have a high precedence. For discussion
and solutions, see: ansible/proposals#21

Removed update-ccache-symlinks.patch because it now has been applied to
ccache in Debian Buster.
JM1 added a commit to JM1/ansible-role-jm1-sudo that referenced this pull request May 8, 2019
Following the split of role 'core' we now depend on role base_system.

Required packages are now installed with virtual packages via role
virtual_pkg. This makes it easier for users to remove ansible-installed
packages later.

Role-specific default values for variables are now only used if these
variables have not been assigned earlier. This requires a workaround
using set_fact because Ansible does not provide a include_defaults like
function yet and role variables have a high precedence. For discussion
and solutions, see: ansible/proposals#21

Added new configuration options uid and shell, see Ansibles user module
for details.
JM1 added a commit to JM1/ansible-role-jm1-container-systemd that referenced this pull request May 8, 2019
…HOME

Following the split of role 'core' we now depend on role base_system.

Required packages are now installed with virtual packages via role
virtual_pkg. This makes it easier for users to remove ansible-installed
packages later.

Role-specific default values for variables are now only used if these
variables have not been assigned earlier. This requires a workaround
using set_fact because Ansible does not provide a include_defaults like
function yet and role variables have a high precedence. For discussion
and solutions, see: ansible/proposals#21

Environment DEVIL_HOME is no longer supported and file
/etc/prepare-container.d/home.sh has been removed because both have
never been intended for general use.
JM1 added a commit to JM1/ansible-role-jm1-container-systemd that referenced this pull request May 8, 2019
…HOME

Following the split of role 'core' we now depend on role base_system.

Required packages are now installed with virtual packages via role
virtual_pkg. This makes it easier for users to remove ansible-installed
packages later.

Role-specific default values for variables are now only used if these
variables have not been assigned earlier. This requires a workaround
using set_fact because Ansible does not provide a include_defaults like
function yet and role variables have a high precedence. For discussion
and solutions, see: ansible/proposals#21

Environment DEVIL_HOME is no longer supported and file
/etc/prepare-container.d/home.sh has been removed because both have
never been intended for general use.
JM1 added a commit to JM1/ansible-role-jm1-container-systemd that referenced this pull request May 8, 2019
…HOME

Following the split of role 'core' we now depend on role base_system.

Required packages are now installed with virtual packages via role
virtual_pkg. This makes it easier for users to remove ansible-installed
packages later.

Role-specific default values for variables are now only used if these
variables have not been assigned earlier. This requires a workaround
using set_fact because Ansible does not provide a include_defaults like
function yet and role variables have a high precedence. For discussion
and solutions, see: ansible/proposals#21

Environment DEVIL_HOME is no longer supported and file
/etc/prepare-container.d/home.sh has been removed because both have
never been intended for general use.
JM1 added a commit to JM1/ansible-role-jm1-dev-hpc that referenced this pull request May 8, 2019
…nstall_*.sh

Following the split of role 'core' we now depend on role base_system.

Required packages are now installed with virtual packages via role
virtual_pkg. This makes it easier for users to remove ansible-installed
packages later.

Role-specific default values for variables are now only used if these
variables have not been assigned earlier. This requires a workaround
using set_fact because Ansible does not provide a include_defaults like
function yet and role variables have a high precedence. For discussion
and solutions, see: ansible/proposals#21

Scripts make_install_elemental.sh, make_install_flame.sh and
make_install_fmt.sh must be run as root, so that passwordless sudo calls
are unnecessary now. Builds are done with user nobody.

Added several scientific apt packages as dependencies.
JM1 added a commit to JM1/ansible-role-jm1-base-system that referenced this pull request May 8, 2019
Detection of fact distribution_codename has been moved to the more
generic role 'common'.

Role-specific default values for variables are now only used if these
variables have not been assigned earlier. This requires a workaround
using set_fact because Ansible does not provide a include_defaults like
function yet and role variables have a high precedence. For discussion
and solutions, see: ansible/proposals#21

Required packages are now installed using virtual packages
jm1-base-system-stage[01] via role virtual_pkg. This makes it easier for
users to remove ansible-installed packages later.

Debconf selections are now handled using Ansible's debconf module.
JM1 added a commit to JM1/ansible-role-jm1-kdevelop that referenced this pull request May 8, 2019
…names

Required packages are now installed with virtual packages via role
virtual_pkg. This makes it easier for users to remove ansible-installed
packages later.

Role-specific default values for variables are now only used if these
variables have not been assigned earlier. This requires a workaround
using set_fact because Ansible does not provide a include_defaults like
function yet and role variables have a high precedence. For discussion
and solutions, see: ansible/proposals#21

Role virtual_pkg does support regex package names, which Ansible's apt
module does not. Thus we reenable regex pattern matches for several qt5
and KDE5/kf5 packages.

kdesrc-build's configuration file .kdesrc-buildrc[.j2] now (a) clones
version-specific branches instead of the master branch to lower the risk
of build failures, (b) does not run tests and (c) uses Ninja instead of
Makefiles.
@lhoss
Copy link

lhoss commented Jun 19, 2019

Being highly motivated to get progress on this topic I

  • shared the issue with @liquidat during last week's Ansible-Automated day in Switzerland/ZH
  • just made my 1. deep-dive into ansible core code (esp. lib/ansible/playbook/role/__init__.py) ..besides (re)assimilating all related proposals and discussions.

The 2 most promising solution approaches IMHO:


Solutions in detail:
(1) overwrite option to include_vars

  • 👍 As the PR shows, it's a very simple change to make this work.
    Also having the new overwrite (or can be named skip_defined see in the PR) could be useful for other use-cases, so IMO getting the PR accepted should be a non-brainer (after fixing conflicts)

(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:

  • 👍 no extra include_vars task required (as in °1)

  • 👍 'natural' support for hash_behaviour=merge. p.eg: a (OS specific)defaults dict var my_dict could be merged with a (higher prio) group_vars my_dict variable

  • 👍 the default_vars attribute could even be enhanced with different modes to:

  • 👎 I could imagine it difficult to combine:

    • loading role defaults during static role init (incl. import_role) a role statically
    • accessing variables (even facts?) like {{ ansible_os_family }} (in the new meta attribute default_vars)
  • 👎 potentially confusing to users to have both the new 'multiple files in defaults' feature (Allow loading dirs from role defaults/vars ansible#36357), but also the new meta default_vars (which can be overseen by role users) would only selectively enable the loading of some files in the defaults folder.


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
Copy link
Member

bcoca commented Jun 20, 2019

we have voted against this several times, latest one is ansible/community#474

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

@bcoca bcoca closed this Jun 20, 2019
@bcoca bcoca added the Rejected label Jun 20, 2019
@lhoss
Copy link

lhoss commented Jun 20, 2019

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants