From 47fc7ab97d4d3a46ec597f1adc98028e84bdaaff Mon Sep 17 00:00:00 2001 From: Jordan Borean Date: Tue, 3 Mar 2020 11:21:17 +1000 Subject: [PATCH] galaxy - Fix collection install dep resolver for bad versions (#67405) (#67413) * Also make sure version is a string and not an int/float (cherry picked from commit 4881af2e7e0506ada0225fd764e874e20569d5b2) --- .../galaxy-collection-install-version.yaml | 2 + lib/ansible/galaxy/collection.py | 25 +++++--- test/units/galaxy/test_collection_install.py | 60 ++++++++++++++++--- 3 files changed, 71 insertions(+), 16 deletions(-) create mode 100644 changelogs/fragments/galaxy-collection-install-version.yaml diff --git a/changelogs/fragments/galaxy-collection-install-version.yaml b/changelogs/fragments/galaxy-collection-install-version.yaml new file mode 100644 index 00000000000..9a56496fa45 --- /dev/null +++ b/changelogs/fragments/galaxy-collection-install-version.yaml @@ -0,0 +1,2 @@ +bugfixes: +- ansible-galaxy - Fix issue when compared installed dependencies with a collection having no ``MANIFEST.json`` or an empty version string in the json diff --git a/lib/ansible/galaxy/collection.py b/lib/ansible/galaxy/collection.py index 33086ab3192..04058d372f9 100644 --- a/lib/ansible/galaxy/collection.py +++ b/lib/ansible/galaxy/collection.py @@ -217,12 +217,15 @@ class CollectionRequirement: requirement = req op = operator.eq - # In the case we are checking a new requirement on a base requirement (parent != None) we can't accept - # version as '*' (unknown version) unless the requirement is also '*'. - if parent and version == '*' and requirement != '*': - break - elif requirement == '*' or version == '*': - continue + # In the case we are checking a new requirement on a base requirement (parent != None) we can't accept + # version as '*' (unknown version) unless the requirement is also '*'. + if parent and version == '*' and requirement != '*': + display.warning("Failed to validate the collection requirement '%s:%s' for %s when the existing " + "install does not have a version set, the collection may not work." + % (to_text(self), req, parent)) + continue + elif requirement == '*' or version == '*': + continue if not op(LooseVersion(version), LooseVersion(requirement)): break @@ -284,7 +287,13 @@ class CollectionRequirement: manifest = info['manifest_file']['collection_info'] namespace = manifest['namespace'] name = manifest['name'] - version = manifest['version'] + version = to_text(manifest['version'], errors='surrogate_or_strict') + + if not hasattr(LooseVersion(version), 'version'): + display.warning("Collection at '%s' does not have a valid version set, falling back to '*'. Found " + "version: '%s'" % (to_text(b_path), version)) + version = '*' + dependencies = manifest['dependencies'] else: display.warning("Collection at '%s' does not have a MANIFEST.json file, cannot detect version." @@ -852,7 +861,7 @@ def _get_collection_info(dep_map, existing_collections, collection, requirement, existing = [c for c in existing_collections if to_text(c) == to_text(collection_info)] if existing and not collection_info.force: # Test that the installed collection fits the requirement - existing[0].add_requirement(to_text(collection_info), requirement) + existing[0].add_requirement(parent, requirement) collection_info = existing[0] dep_map[to_text(collection_info)] = collection_info diff --git a/test/units/galaxy/test_collection_install.py b/test/units/galaxy/test_collection_install.py index b995177fca7..74101fb18c1 100644 --- a/test/units/galaxy/test_collection_install.py +++ b/test/units/galaxy/test_collection_install.py @@ -165,13 +165,14 @@ def test_build_requirement_from_path(collection_artifact): assert actual.dependencies == {} -def test_build_requirement_from_path_with_manifest(collection_artifact): +@pytest.mark.parametrize('version', ['1.1.1', 1.1, 1]) +def test_build_requirement_from_path_with_manifest(version, collection_artifact): manifest_path = os.path.join(collection_artifact[0], b'MANIFEST.json') manifest_value = json.dumps({ 'collection_info': { 'namespace': 'namespace', 'name': 'name', - 'version': '1.1.1', + 'version': version, 'dependencies': { 'ansible_namespace.collection': '*' } @@ -188,8 +189,8 @@ def test_build_requirement_from_path_with_manifest(collection_artifact): assert actual.b_path == collection_artifact[0] assert actual.api is None assert actual.skip is True - assert actual.versions == set([u'1.1.1']) - assert actual.latest_version == u'1.1.1' + assert actual.versions == set([to_text(version)]) + assert actual.latest_version == to_text(version) assert actual.dependencies == {'ansible_namespace.collection': '*'} @@ -203,6 +204,42 @@ def test_build_requirement_from_path_invalid_manifest(collection_artifact): collection.CollectionRequirement.from_path(collection_artifact[0], True) +def test_build_requirement_from_path_no_version(collection_artifact, monkeypatch): + manifest_path = os.path.join(collection_artifact[0], b'MANIFEST.json') + manifest_value = json.dumps({ + 'collection_info': { + 'namespace': 'namespace', + 'name': 'name', + 'version': '', + 'dependencies': {} + } + }) + with open(manifest_path, 'wb') as manifest_obj: + manifest_obj.write(to_bytes(manifest_value)) + + mock_display = MagicMock() + monkeypatch.setattr(Display, 'display', mock_display) + + actual = collection.CollectionRequirement.from_path(collection_artifact[0], True) + + # While the folder name suggests a different collection, we treat MANIFEST.json as the source of truth. + assert actual.namespace == u'namespace' + assert actual.name == u'name' + assert actual.b_path == collection_artifact[0] + assert actual.api is None + assert actual.skip is True + assert actual.versions == set(['*']) + assert actual.latest_version == u'*' + assert actual.dependencies == {} + + assert mock_display.call_count == 1 + + actual_warn = ' '.join(mock_display.mock_calls[0][1][0].split('\n')) + expected_warn = "Collection at '%s' does not have a valid version set, falling back to '*'. Found version: ''" \ + % to_text(collection_artifact[0]) + assert expected_warn in actual_warn + + def test_build_requirement_from_tar(collection_artifact): actual = collection.CollectionRequirement.from_tar(collection_artifact[1], True, True) @@ -497,13 +534,20 @@ def test_add_collection_requirements(versions, requirement, expected_filter, exp assert req.latest_version == expected_latest -def test_add_collection_requirement_to_unknown_installed_version(): +def test_add_collection_requirement_to_unknown_installed_version(monkeypatch): + mock_display = MagicMock() + monkeypatch.setattr(Display, 'display', mock_display) + req = collection.CollectionRequirement('namespace', 'name', None, 'https://galaxy.com', ['*'], '*', False, skip=True) - expected = "Cannot meet requirement namespace.name:1.0.0 as it is already installed at version 'unknown'." - with pytest.raises(AnsibleError, match=expected): - req.add_requirement(str(req), '1.0.0') + req.add_requirement('parent.collection', '1.0.0') + assert req.latest_version == '*' + + assert mock_display.call_count == 1 + + actual_warn = ' '.join(mock_display.mock_calls[0][1][0].split('\n')) + assert "Failed to validate the collection requirement 'namespace.name:1.0.0' for parent.collection" in actual_warn def test_add_collection_wildcard_requirement_to_unknown_installed_version():