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

Force collections to be static #68723

Merged
merged 6 commits into from Apr 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelogs/fragments/68723-force-static-collections.yaml
@@ -0,0 +1,3 @@
bugfixes:
- Force collection names to be static so that a warning is generated because
templating currently does not work (see https://github.com/ansible/ansible/issues/68704).
16 changes: 15 additions & 1 deletion lib/ansible/playbook/collectionsearch.py
Expand Up @@ -7,6 +7,10 @@
from ansible.module_utils.six import string_types
from ansible.playbook.attribute import FieldAttribute
from ansible.utils.collection_loader import AnsibleCollectionLoader
from ansible.template import is_template, Environment
from ansible.utils.display import Display

display = Display()


def _ensure_default_collection(collection_list=None):
Expand All @@ -32,7 +36,8 @@ def _ensure_default_collection(collection_list=None):
class CollectionSearch:

# this needs to be populated before we can resolve tasks/roles/etc
_collections = FieldAttribute(isa='list', listof=string_types, priority=100, default=_ensure_default_collection)
_collections = FieldAttribute(isa='list', listof=string_types, priority=100, default=_ensure_default_collection,
always_post_validate=True, static=True)

def _load_collections(self, attr, ds):
# this will only be called if someone specified a value; call the shared value
Expand All @@ -41,4 +46,13 @@ def _load_collections(self, attr, ds):
if not ds: # don't return an empty collection list, just return None
return None

# This duplicates static attr checking logic from post_validate()
# because if the user attempts to template a collection name, it will
# error before it ever gets to the post_validate() warning.
env = Environment()
for collection_name in ds:
if is_template(collection_name, env):
display.warning('"collections" is not templatable, but we found: %s, '
'it will not be templated and will be used "as is".' % (collection_name))

Shrews marked this conversation as resolved.
Show resolved Hide resolved
return ds
38 changes: 38 additions & 0 deletions test/units/playbook/test_collectionsearch.py
@@ -0,0 +1,38 @@
# (c) 2020 Ansible Project
#
# This file is part of Ansible
#
# Ansible is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# Ansible is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with Ansible. If not, see <http://www.gnu.org/licenses/>.

from __future__ import (absolute_import, division, print_function)
__metaclass__ = type

from ansible.playbook.collectionsearch import CollectionSearch

import pytest


def test_collection_static_warning(capsys):
"""Test that collection name is not templated.

Also, make sure that users see the warning message for the referenced name.
"""

collection_name = 'foo.{{bar}}'
cs = CollectionSearch()
assert collection_name in cs._load_collections(None, [collection_name])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does that method return? Can we have an equality check here?
Also, is there a public method that could be called instead of the private one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It returns a list of collection names and is adding 'ansible.legacy' to that list. I'm not aware of a public method to call instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the order is deterministic, you could have a more precise check here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, in this test case, it should always be the first element based on the current code. But in this case, we don't actually care about positioning, just that it appears anywhere in the list, independent of which other collections are inserted. Do you feel always checking against the first element is better? That could open it up to test failures if we later decide to alter the ordering for some reason that I cannot yet foresee.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think either way is fine. But in general, I prefer tests to be as precise as possible to prevent some buggy behavior from squeezing in just because it has similar properties.


std_out, std_err = capsys.readouterr()
assert '[WARNING]: "collections" is not templatable, but we found: %s' % collection_name in std_err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use equality which would be more precise and replace that check that you had earlier with calls count == 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was using IN here because it seems the capsys output contains color-coding chars and newlines so IN was easier to use for confirming the main data here (warning + collection name) is present.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, you could probably monkeypatch ANSIBLE_COLOR:

if ANSIBLE_COLOR:
.

But I looked closer at the display function and it looks like it uses an actual logger from logging stdlib module (

logger.log(lvl, msg2)
). So it's even easier than I thought initially.

You should be able to use the caplog fixture. Let me see if I can quickly come up with a patch.

assert '' == std_out
Shrews marked this conversation as resolved.
Show resolved Hide resolved