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

Allow loading dirs from role defaults/vars #36357

Merged
merged 1 commit into from
Apr 10, 2018

Conversation

agaffney
Copy link
Contributor

@agaffney agaffney commented Feb 18, 2018

SUMMARY

This commit moves code to look for vars files/dirs to a common place and uses it for loading role defaults/vars. This allows things such as defaults/main or vars/main being a directory in a role, allowing splitting of defaults/vars into multiple files. This commit also fixes the role loading unit tests for py3 when bytestrings are used for paths instead of utf8 strings.

This fixes #14248 and #11639.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

vars loading

ANSIBLE VERSION

N/A

ADDITIONAL INFORMATION

N/A

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. feature_pull_request 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 Feb 18, 2018
@agaffney agaffney force-pushed the role_defaults_vars_dirs branch 4 times, most recently from c3b541b to 7efca52 Compare February 18, 2018 07:02
@ansibot ansibot added the test This PR relates to tests. label Feb 18, 2018
@agaffney agaffney force-pushed the role_defaults_vars_dirs branch 2 times, most recently from e4eb6b1 to d4aabda Compare February 18, 2018 14:35
@agaffney agaffney changed the title [WIP] Generalize loading vars files from dir (fixes #14248 and #11639) [WIP] Allow loading dirs from role defaults/vars (fixes #14248 and #11639) Feb 18, 2018
@agaffney agaffney force-pushed the role_defaults_vars_dirs branch 2 times, most recently from 94e363f to 06dcc77 Compare February 18, 2018 14:38
@agaffney agaffney force-pushed the role_defaults_vars_dirs branch 4 times, most recently from d090018 to 2fdaafc Compare February 18, 2018 15:08
@agaffney
Copy link
Contributor Author

@bcoca

@agaffney agaffney force-pushed the role_defaults_vars_dirs branch 2 times, most recently from d80f63c to a696cdd Compare February 19, 2018 01:32
@agaffney agaffney changed the title [WIP] Allow loading dirs from role defaults/vars (fixes #14248 and #11639) [WIP] Allow loading dirs from role defaults/vars Feb 19, 2018
@agaffney agaffney force-pushed the role_defaults_vars_dirs branch 2 times, most recently from 7e44e00 to c8c1b0e Compare February 19, 2018 13:02
@ryansb ryansb removed the needs_triage Needs a first human triage before being processed. label Feb 20, 2018
@agaffney agaffney changed the title [WIP] Allow loading dirs from role defaults/vars Allow loading dirs from role defaults/vars Feb 22, 2018
@ansibot ansibot removed the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Feb 22, 2018
@agaffney
Copy link
Contributor Author

agaffney commented Jun 3, 2018

Yes, that should probably be continue. I work in a lot of different languages and those kinds of things are easy to mix up. That code path must not be exercised much, since it didn't break any of the tests and nobody has screamed yet.

@agaffney
Copy link
Contributor Author

agaffney commented Jun 3, 2018

I'm having a hard time getting ansible to even hit that line of code. It should only get there on a role subdir such as tasks/ that uses allow_dir=False and when a matching directory is found before a file, but I can't get it to fail when I add a unit test like that.

@andriilahuta
Copy link
Contributor

andriilahuta commented Jun 3, 2018

I'm not familiar with ansible internals, but this seemed to do the trick:

    @patch('ansible.playbook.role.definition.unfrackpath', mock_unfrackpath_noop)
    def test_load_role_with_tasks_dir_vs_file(self):

        fake_loader = DictDataLoader({
            "/etc/ansible/roles/foo_tasks/tasks/custom_main/foo.yml": """
            - command: bar
            """,
            "/etc/ansible/roles/foo_tasks/tasks/custom_main.yml": """
            - command: baz
            """,
        })

        mock_play = MagicMock()
        mock_play.ROLE_CACHE = {}

        i = RoleInclude.load('foo_tasks', play=mock_play, loader=fake_loader)
        r = Role.load(i, play=mock_play, from_files=dict(tasks='custom_main'))

        self.assertEqual(r._task_blocks[0]._ds[0]['command'], 'baz')

@agaffney
Copy link
Contributor Author

agaffney commented Jun 3, 2018

Yeah, that seems to trigger the bug (or something that could be attributed to it). I did something very similar, but I wasn't using tasks_from, which appears to be necessary to trigger it.

agaffney added a commit to agaffney/ansible that referenced this pull request Jun 3, 2018
Also add tests around that code path
amenonsen pushed a commit that referenced this pull request Jun 7, 2018
Also add tests around that code path
andriilahuta added a commit to andriilahuta/ansible that referenced this pull request Jun 20, 2018
maxamillion pushed a commit to maxamillion/ansible that referenced this pull request Jun 21, 2018
mattclay pushed a commit that referenced this pull request Jun 22, 2018
Also add tests around that code path
jacum pushed a commit to jacum/ansible that referenced this pull request Jun 26, 2018
kbreit pushed a commit to kbreit/ansible that referenced this pull request Jul 3, 2018
andriilahuta added a commit to andriilahuta/ansible that referenced this pull request Jul 20, 2018
mattclay pushed a commit that referenced this pull request Jul 27, 2018
ilicmilan pushed a commit to ilicmilan/ansible that referenced this pull request Nov 7, 2018
This commit moves code to look for vars files/dirs to a common place and
uses it for loading role defaults/vars. This allows things such as
'defaults/main' or 'vars/main' being a directory in a role, allowing
splitting of defaults/vars into multiple files. This commit also fixes
the role loading unit tests for py3 when bytestrings are used for paths
instead of utf8 strings.

This fixes ansible#14248 and ansible#11639.
@luckylittle
Copy link
Contributor

It doesn't really work as i would expect it to work (why does 3rd scenario not work?). I tested different scenarios and this is what i found:

  1. Doesn't work:
defaults/file1.yml
defaults/file2.yml
defaults/main.yml
  1. Doesn't work:
defaults/file1.yml
defaults/file2.yml
defaults/file3.yml
  1. Doesn't work:
defaults/main/file1.yml
defaults/main/file2.yml
defaults/main.yml
  1. Works:
defaults/main/file1.yml
defaults/main/file2.yml
defaults/main/main.yml

If this is intended, i am happy to update Playbooks Best Practices.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature This issue/PR relates to a feature request. python3 stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple files in role's defaults directory
6 participants