Don't ignore invalid collection entries in requirements.yml #83715

cavcrosby · 2024-08-03T21:36:38Z

SUMMARY

ISSUE TYPE

Bugfix Pull Request

felixfontein · 2024-08-06T07:53:06Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

        req_signature_sources = collection_req.get('signatures', None)
+        if frozenset(collection_req.keys()).difference(
+            frozenset({'name', 'version', 'type', 'source', 'signatures'})
+        ):


Maybe store the resulting difference in a variable and mention the invalid keys in the error message. That makes it easier for users to find the bad entries.

Also you're repeating the valid keys twice, here and in the error message. How about storing them also in a variable so they are only listed in one place?

felixfontein · 2024-08-06T16:55:29Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

+                "The keys (%s) are not valid when installing collection "
+                "requirement entries. Be sure to only use the following "
+                "supported keys (%s)."
+                % (', '.join(key_diff), ', '.join(SUPPORTED_REQUIREMENTS_KEYS))


I would personally sort the sets before joining them so that the error message becomes more deterministic. (It makes it also easier to scan the list of supported keys if they are always in alphabetical order :) .)

lib/ansible/galaxy/dependency_resolution/dataclasses.py

cavcrosby · 2024-08-09T18:34:03Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

+
        if req_type is None:
-            if (  # FIXME: decide on the future behavior:
-                    _ALLOW_CONCRETE_POINTER_IN_SOURCE


My interpretation of this block is that this checks if the source key points to an artifact. Since the desired behavior is that the source key shouldn't point to an artifact, I thought this block was no longer necessary. Perhaps I'm wrong and this should stay in?

I don't remember why this was added. Inspecting 595413d / #72591 does not shed any light on that. Perhaps @s-hertel, @jborean93 and I talked about it on our weekly calls. But I doubt they'll remember discussions that were taking place over 4 years ago.

But looking at it now, I'd assume that we didn't want people to put in ambiguous things into this field without specifying the type explicitly. And since there's this always-false feature flag _ALLOW_CONCRETE_POINTER_IN_SOURCE, it was probably something that might've been seen as controversial at the time.

This check seems to prevent assuming the galaxy type (the following elif branch) if something looks like a path to an artifact.

@cavcrosby Yeah, that's my interpretation too. I think it's fine to remove this for now. There is a UX issue, but the solution hasn't been agreed upon.

@webknjaz I don't know exactly, but I think the FIXME is related to the conversation in our project channel on Oct 21 (Oct 22 for @jborean93). You had suggested making source an alias of name, and removing its previous function in favor of simplicity. I was open to the UX changes in a future PR, as long as name and source were mutually exclusive for the new behavior, since otherwise it would cause a new UX issue. @jborean93 pointed out that the existing source feature couldn't just be removed, so I believe we agreed on keeping parity with the existing requirement file syntax in #72591 since the dependency resolver didn't require those changes. We already had too many untested permutations so I created https://gist.github.com/s-hertel/4cb449fb9407865ae515d592ff1969c8 (all valid permutations at that time) and based some tests off of it, but unfortunately backwards compatibility tests weren't sufficient to prevent new undocumented permutations from sneaking in. It looks like source is now sort of an alias of name (and not mutually exclusive) in addition to its original function (at the expense of unhandled tracebacks that wouldn't occur by holding the feature correctly). I think part of the issue is this method is overly complex, and obfuscated that a user-facing change was made.

I don't think we should overload source with name functionality while it is a distinct and separate feature. I think we should rename source to server and deprecate source as an alias after a couple releases. Not in this PR, for clarity. That will fix the confusion between name and source (galaxy server), especially as compared to roles since they have a name and src (pointer to specific role). In general, I'm in favor of roles adopting collection limitations/behaviors rather than the other way around. So we should add a source/server option to role requirements instead. ;) (kidding, sort of)

Cool, yea I do think server would be a less ambiguous key name.

Agreed that this method has high complexity. I remember struggling with making it simpler initially, but the priorities were on making the resolver work rather than getting stuck on this bit IIRC, so refactoring got postponed.

cavcrosby · 2024-08-09T18:34:59Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

-        req_type = collection_req.get('type')
+        result = ArgumentSpecValidator(
+            argument_spec=dict(
+                name=dict(type='str', required=True),


Currently, seeing as collection_req is always passed in as a dictionary, I have set up the argument_spec to always require a name key. I could toggle this if collection_req was instead a list of collection names, but I'm not sure where that would get used?

You are correct. Users can pass in a requirements file that contains a list of strings, but this method will only receive the normalized dict.

might be a good time to add typing info to reflect this...

@cavcrosby in case of bare strings, the from_string() method above is the entry point that eventually calls this one with a dict.

cavcrosby · 2024-08-09T18:40:45Z

test/integration/targets/ansible-galaxy-collection/tasks/pinned_pre_releases_in_deptree.yml

      ansible-galaxy collection install
      meta_ns_with_transitive_wildcard_dep.meta_name_with_transitive_wildcard_dep
-      rc_meta_ns_with_transitive_dev_dep.rc_meta_name_with_transitive_dev_dep:=2.4.5-rc5
+      rc_meta_ns_with_transitive_dev_dep.rc_meta_name_with_transitive_dev_dep:==2.4.5-rc5


This change was to have the ansible-galaxy-collection integration tests pass. According to the docs, == should be used to install pre-release candidates, but I'd presume this has worked up to this point?

I think both single and double equals has historically been functional in both roles and collections. If there's a test, this means that we expected it to work.

cavcrosby · 2024-11-11T17:52:29Z

Circling back around, mind giving my most recent changes a look @felixfontein @s-hertel?

webknjaz · 2024-11-15T01:31:28Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

+            req_version and req_type == 'subdirs'
+        ):
+            raise AnsibleError(
+                f"The {req_name} 'version' key is not applicable when 'type' is set to 'subdirs'."


I think this is an internal type that is not even exposed to the end-users. So showing it like this would probably be confusing.

Is that the case even as there is an explicit mention of this type in the docs?

@cavcrosby You are correct, this is exposed to end users via the requirements file. It can also be inferred from the requirement name. I think we should give the same error whether the type is inferred or not - we just need to include enough details that the user can determine which requirement caused the error even if they didn't explicitly provide the type.

Honestly, I'm not even sure if subdirs was supposed to be documented. @s-hertel do you remember why you approved #75872? IIRC, it's just used as an intermediate thing in the depresolver to deal with Git URLs that don't have collection layout on the top level… I'd say it's an implementation detail rather than something people should be allowed to put in requirements.yml.

I was against adding both type dir and subdirs, but outvoted iirc. We had agreed intentional changes needed to be represented in the changelog. The major_changes mention installing namespaces:

It became possible to install Ansible Collections from local folders and namespaces folder similar to SCM structure with multiple collections.

Fwiw, I think we should deprecate git, dir, and subdirs.

@webknjaz I corrected that documentation in ansible/ansible-documentation#2259 (comment), and explained I wasn't sure why I approved it.

I think type=git is something very common in the wild. Deprecating that would upset very many users.

Fwiw, I think we should deprecate git, dir, and subdirs.

I don't mind git, I think. It makes it possible to disambiguate a URL to an archive file accessible over HTTP vs. Git accessible over HTTP. But dir is probably semantically close to file. subdirs is what bothers me most.

Fwiw, I think we should deprecate git, dir, and subdirs.

I didn't realize that I documented installing from namespace folders back then… I think subdirs shouldn't be allowed in the requirements file schema, but internally, it was needed for what we called “virtual” collections. Whether to allow it on the CLI is also controversial.
I think most such validation should happen where the end-user inputs things (CLI and requirements files) but not in the constructors of our dataclasses.

webknjaz · 2024-11-15T01:35:34Z

test/units/galaxy/test_collection_install.py

+    tmp_path = os.path.join(os.path.split(collection_artifact[1])[0], b'temp')
+    concrete_artifact_cm = collection.concrete_artifact_manager.ConcreteArtifactsManager(tmp_path, validate_certs=False)
+    expected = (
+        "The foo 'source' key is not applicable when 'name' is not a FQCN."


It's a good convention to always use raw-strings for regexes:

Suggested change

"The foo 'source' key is not applicable when 'name' is not a FQCN."

r"The foo 'source' key is not applicable when 'name' is not a FQCN\."

Also, . matches any char, hence the escaping.

s-hertel

Sorry for not re-reviewing sooner. This method is always difficult to grok and I kept getting stuck debugging the suggestions I wanted to give.

s-hertel · 2025-01-24T18:25:21Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

+                name=dict(type='str', required=True),
+                signatures=dict(type='list', elements='str'),
+                source=dict(type='raw'),
+                type=dict(type='str'),


Suggested change

type=dict(type='str'),

type=dict(type='str', choices=['file', 'galaxy', 'git', 'url', 'dir', 'subdirs']),

s-hertel · 2025-01-24T18:31:46Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

+            error = result.errors[0]
+            if isinstance(error, UnsupportedError):
+                raise AnsibleError(
+                    "The following {collection_name!s} collection requirement entry keys are not "
+                    "valid: {unsupported_params!s}".format(
+                        collection_name=req_name,
+                        unsupported_params=', '.join(sorted(result.unsupported_parameters))
+                    )
+                )
+            raise AnsibleError(
+                f"Failed to install collection requirement entry: {error.msg}"
+            )


Rather than special-casing the first failure or error type, this should display all errors. I would also not mention install specifically, since there are multiple subcommands that use this method.

Suggested change

error = result.errors[0]

if isinstance(error, UnsupportedError):

raise AnsibleError(

"The following {collection_name!s} collection requirement entry keys are not "

"valid: {unsupported_params!s}".format(

collection_name=req_name,

unsupported_params=', '.join(sorted(result.unsupported_parameters))

)

)

raise AnsibleError(

f"Failed to install collection requirement entry: {error.msg}"

)

raise AnsibleError(

f"Failed to parse collection requirement entry: {', '.join(result.error_messages)}"

)

If there's an issue with unsupported parameters being randomly ordered (not sure if there is), that could be fixed in the argument spec validator.

Agreed on omitting the subcommand in the error message. I believe I special-cased the first error type because the error message returned wouldn't read the best when appended. For example, if the foo and bar keys are used as part of a requirement, the exception raised will have the message ERROR! Failed to parse collection requirement entry: bar, foo. Supported parameters include: name, signatures, source, type, version..

Good point, it doesn't really include context about which requirement specifies the unsupported keys.

How about ERROR! Failed to parse collection requirement entry: {'name': 'amazon.aws', 'version': '6.0.0', 'foo': 'baz', 'bar': 'qux'}: bar, foo. Supported parameters include: name, signatures, source, type, version.?

if result.error_messages: raise AnsibleError( f"Failed to parse collection requirement entry: {collection_req}: {', '.join(result.error_messages)}" )

Yea, that'll work.

s-hertel · 2025-01-24T18:33:22Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

-        req_type = collection_req.get('type')
+        result = ArgumentSpecValidator(
+            argument_spec=dict(
+                name=dict(type='str', required=True),


You are correct. Users can pass in a requirements file that contains a list of strings, but this method will only receive the normalized dict.

s-hertel · 2025-01-24T18:34:14Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

+            req_version and req_type == 'subdirs'
+        ):
+            raise AnsibleError(
+                f"The {req_name} 'version' key is not applicable when 'type' is set to 'subdirs'."


@cavcrosby You are correct, this is exposed to end users via the requirements file. It can also be inferred from the requirement name. I think we should give the same error whether the type is inferred or not - we just need to include enough details that the user can determine which requirement caused the error even if they didn't explicitly provide the type.

s-hertel · 2025-01-24T18:38:41Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

+
        if req_type is None:
-            if (  # FIXME: decide on the future behavior:
-                    _ALLOW_CONCRETE_POINTER_IN_SOURCE


@cavcrosby Yeah, that's my interpretation too. I think it's fine to remove this for now. There is a UX issue, but the solution hasn't been agreed upon.

@webknjaz I don't know exactly, but I think the FIXME is related to the conversation in our project channel on Oct 21 (Oct 22 for @jborean93). You had suggested making source an alias of name, and removing its previous function in favor of simplicity. I was open to the UX changes in a future PR, as long as name and source were mutually exclusive for the new behavior, since otherwise it would cause a new UX issue. @jborean93 pointed out that the existing source feature couldn't just be removed, so I believe we agreed on keeping parity with the existing requirement file syntax in #72591 since the dependency resolver didn't require those changes. We already had too many untested permutations so I created https://gist.github.com/s-hertel/4cb449fb9407865ae515d592ff1969c8 (all valid permutations at that time) and based some tests off of it, but unfortunately backwards compatibility tests weren't sufficient to prevent new undocumented permutations from sneaking in. It looks like source is now sort of an alias of name (and not mutually exclusive) in addition to its original function (at the expense of unhandled tracebacks that wouldn't occur by holding the feature correctly). I think part of the issue is this method is overly complex, and obfuscated that a user-facing change was made.

I don't think we should overload source with name functionality while it is a distinct and separate feature. I think we should rename source to server and deprecate source as an alias after a couple releases. Not in this PR, for clarity. That will fix the confusion between name and source (galaxy server), especially as compared to roles since they have a name and src (pointer to specific role). In general, I'm in favor of roles adopting collection limitations/behaviors rather than the other way around. So we should add a source/server option to role requirements instead. ;) (kidding, sort of)

s-hertel · 2025-01-24T18:42:44Z

test/integration/targets/ansible-galaxy-collection-scm/tasks/requirements.yml

    src: "{{ item }}"
    dest: "{{ galaxy_dir }}/requirements/{{ item }}"
  loop:
-    - source_only.yml


It's fine if the error message is updated, but this scenario should still be an error and have test coverage. I think there's still a bug in from_requirement_dict conflating source with name, which is why this test fails. It needs some kind of check like:

if req_type != 'galaxy' and req_source is not None: raise AnsibleError( f"Invalid source found for the collection {req_name}: {req_source}. " f"The 'source' key is not applicable when 'name' ({req_name}) is not a FQCN or the 'type' ({req_type}) is not galaxy." )

I believe the error that now occurs with the test is that the requirement in the source_only.yml file is missing the name key. The following here shows the updated error message before removing the test.

Ah, that makes sense. The new error looks good. 👍 I would still keep the integration coverage.

s-hertel · 2025-01-24T18:45:55Z

test/units/galaxy/test_collection_install.py

+        Requirement.from_requirement_dict({'name': to_text(collection_artifact[1]), 'invalid_key': 'foo', 'key': 'bar'}, concrete_artifact_cm)
+
+
+def test_build_requirement_from_tar_invalid_name_with_source(collection_artifact):


Minor nit: all the new tests following and including this one should not include from_tar in the name, since the name is not a path to a tarfile and the type is not explicitly specified as file.

Suggested change

def test_build_requirement_from_tar_invalid_name_with_source(collection_artifact):

def test_build_requirement_invalid_name_with_source(collection_artifact):

s-hertel · 2025-01-24T18:48:34Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

+                f"The {req_name} 'source' key is not applicable when 'name' is not a FQCN."
+            )
+
        if req_type is None:


I think this needs to happen earlier - we need to infer the type if it is unspecified, and then use the type to perform the type-specific option combination validation for source (only applicable for type: galaxy), version (not applicable for type: subdirs), signatures (only applicable for type: galaxy).

s-hertel · 2025-01-24T19:33:38Z

changelogs/fragments/83715-raise-on-invalid-collection-entry.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - ansible-galaxy collection install - raise errors on invalid collection entry in requirements.yml (https://github.com/ansible/ansible/issues/83699).


Suggested change

- ansible-galaxy collection install - raise errors on invalid collection entry in requirements.yml (https://github.com/ansible/ansible/issues/83699).

- ansible-galaxy collection install|download|verify - raise errors on invalid collection entry in requirements.yml (https://github.com/ansible/ansible/issues/83699).

Maybe also highlight things:

Suggested change

- ansible-galaxy collection install - raise errors on invalid collection entry in requirements.yml (https://github.com/ansible/ansible/issues/83699).

- ``ansible-galaxy collection install|download|verify`` - raise errors on invalid collection entry in :file:`requirements.yml` (https://github.com/ansible/ansible/issues/83699).

I think :file: is a Sphinx extension, and not something supported natively by docutils.

Fair. Although, I don't think GH even uses docutils. Their parser is really dumb.

s-hertel · 2025-01-24T19:43:15Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

+        if req_type != 'git' and req_version != '*':
+            if not HAS_PACKAGING:
+                raise AnsibleError("Failed to import packaging, check that a supported version is installed")
+
+            try:
+                specifier = next(iter(
+                    SpecifierSet(
+                        '=' + req_version
+                        if req_version.startswith('=') and req_version[:2] != '=='
+                        else req_version
+                    )
+                ))
+            except InvalidSpecifier:
+                specifier = None
+
+            try:
+                version = req_version if not specifier else specifier.version
+                SemanticVersion(version)
+            except ValueError:
+                raise AnsibleError(
+                    f"The {req_name} 'version' key must be a valid collection version "
+                    "(see specification at https://semver.org/)."
+                )


After thinking about this more and looking at pip for comparison, I'm not totally sure about this change, since it is not backwards compatible and requirements don't have to be SemVer for comparison against a SemVer collection version. pip install "botocore==1" installs botocore 1.0.0. Maybe the SemanticVersion check in providers.py is sufficient, and no version validation should be done here (besides giving an error for version + type subdirs, since it is not applicable).

I am curious as to what issues you've seen with the version key being something aside a valid semantic version. Looking at this again, if this isn't an official requirement for all collections, this could be softly enforced by something like an ansible-lint opt-in rule.

The main issue is non-str versions (#78067). That bug can occur with any requirement type except git and subdirs on recent versions.

$ ansible-galaxy collection install amazon.aws:6 # installs 6.0.0

collections: - name: amazon.aws version: 6 # causes unhandled traceback

Strings that are invalid semantic versions also cause a traceback:

ERROR! Unexpected Exception, this is probably a bug: Non integer values in LooseVersion ('a1.b2') the full traceback was: Traceback (most recent call last): File "/home/shertel/ansible/lib/ansible/cli/__init__.py", line 656, in cli_executor exit_code = cli.run() ^^^^^^^^^ File "/home/shertel/ansible/bin/ansible-galaxy", line 730, in run return context.CLIARGS['func']() ^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/shertel/ansible/bin/ansible-galaxy", line 98, in method_wrapper return wrapped_method(*args, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/shertel/ansible/bin/ansible-galaxy", line 1395, in execute_install self._execute_install_collection( File "/home/shertel/ansible/bin/ansible-galaxy", line 1444, in _execute_install_collection install_collections( File "/home/shertel/ansible/lib/ansible/galaxy/collection/__init__.py", line 703, in install_collections unsatisfied_requirements -= set() if force or force_deps else { ^ File "/home/shertel/ansible/lib/ansible/galaxy/collection/__init__.py", line 707, in <setcomp> if req.fqcn == exs.fqcn and meets_requirements(exs.ver, req.ver) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/shertel/ansible/lib/ansible/galaxy/dependency_resolution/versioning.py", line 62, in meets_requirements SemanticVersion.from_loose_version(LooseVersion(requirement)), ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/shertel/ansible/lib/ansible/utils/version.py", line 174, in from_loose_version raise ValueError("Non integer values in %r" % loose_version) ValueError: Non integer values in LooseVersion ('a1.b2')

But I think it's less of an issue, since it should fail either way. The extra validation here would really just be to present a better error for that second case. I do think it's worth validating this in ansible-galaxy so if you want to keep it in this PR, don't let me deter you (but could be done in another PR too, if you want to keep this one simple).

To maintain backwards compatibility, the requirement version just needs to be usable by meets_requirement (https://github.com/ansible/ansible/blob/devel/lib/ansible/galaxy/dependency_resolution/versioning.py#L23). So the version should be split on ,, and then each segment could be validated as a SemanticVersion.from_loose_version (after removing the operator from that segment).

webknjaz · 2025-01-30T19:29:18Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

+
+            try:
+                specifier = next(iter(
+                    SpecifierSet(


I don't think that such version conversions belong in this module. There's ansible.utils.version that could hold a helper.

webknjaz · 2025-01-30T19:41:07Z

test/units/galaxy/test_collection_install.py

+    except AnsibleError as err:
+        pytest.fail(f'Failed to handle a collection with a pre-release version: {err}')


I don't think that this try/except dance is necessary. Pytest would surface the exception regardless. It'd be enough to add a code comment that this is a smoke test checking for no exceptions.

…83699)

webknjaz · 2025-03-24T17:35:33Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

+                    SemanticVersion.from_loose_version(LooseVersion(segment))
+                except ValueError:
+                    raise AnsibleError(
+                        f"The {req_name} 'version' key is not a valid version."


A little bit more detail would make this more informative to the end-users:

Suggested change

f"The {req_name} 'version' key is not a valid version."

f"The {req_name} 'version' key is not a valid version: "

f"'{segment}' part of '{req_version}' does not follow SemVer."

webknjaz · 2025-03-24T17:48:50Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

+            for segment in req_version.split(','):
+                if segment[:2] in OP_MAP:
+                    segment = segment[2:]
+                elif segment[0] in OP_MAP:
+                    segment = segment[1:]
+                try:
+                    SemanticVersion.from_loose_version(LooseVersion(segment))
+                except ValueError:
+                    raise AnsibleError(
+                        f"The {req_name} 'version' key is not a valid version."
+                    )


Could you move the for-loop plus check into versioning.py? It already has related code, and it'd be better semantically to keep it close.
Additionally, it would reduce the level of nesting here. This method is complex enough. Just compare with this:

Suggested change

for segment in req_version.split(','):

if segment[:2] in OP_MAP:

segment = segment[2:]

elif segment[0] in OP_MAP:

segment = segment[1:]

try:

SemanticVersion.from_loose_version(LooseVersion(segment))

except ValueError:

raise AnsibleError(

f"The {req_name} 'version' key is not a valid version."

)

try:

assert_requrements_valid(req_version)

except ValueError:

raise AnsibleError(

f"The {req_name} 'version' key is not a valid version: "

f"'{segment}' part of '{req_version}' does not follow SemVer."

)

webknjaz · 2025-03-24T17:52:05Z

changelogs/fragments/83715-raise-on-invalid-collection-entry.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - "``ansible-galaxy collection install|download|verify`` - raise errors on invalid collection entry in `requirements.yml` (https://github.com/ansible/ansible/issues/83699)."


Suggested change

- "``ansible-galaxy collection install|download|verify`` - raise errors on invalid collection entry in `requirements.yml` (https://github.com/ansible/ansible/issues/83699)."

- "``ansible-galaxy collection install|download|verify`` - raise errors on invalid collection entry in ``requirements.yml`` (https://github.com/ansible/ansible/issues/83699)."

webknjaz · 2025-03-24T17:54:54Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

+            argument_spec=dict(
+                name=dict(type='str', required=True),
+                signatures=dict(type='list', elements='str'),
+                source=dict(type='raw'),


Why not str?

Suggested change

source=dict(type='raw'),

source=dict(type='str'),

webknjaz · 2025-03-24T17:56:31Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

-        req_version = collection_req.get('version', '*')
-        req_type = collection_req.get('type')
+    def from_requirement_dict(cls, collection_req: dict, art_mgr: ConcreteArtifactsManager, validate_signature_options: bool = True):
+        DEFAULT_VERSION = '*'


This variable name is misleading. * means “any”. There's no default I can think of.

Suggested change

DEFAULT_VERSION = '*'

ANY_VERSION_SPEC = '*'

Also, if this is being made into an upper-case constant, it's probably best to move it to the top of the module as _ANY_VERSION_SPEC.

webknjaz · 2025-03-24T17:59:13Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

-        req_type = collection_req.get('type')
+    def from_requirement_dict(cls, collection_req: dict, art_mgr: ConcreteArtifactsManager, validate_signature_options: bool = True):
+        DEFAULT_VERSION = '*'
+        result = ArgumentSpecValidator(


Could you rename result to a meaningful variable name? It should carry some context and not be overly generic.

Also, let's assign ArgumentSpecValidator() to something module-global, as the spec validator does not really need to be recreated when initializing every single requirement object.

webknjaz · 2025-03-24T18:00:07Z

lib/ansible/galaxy/dependency_resolution/dataclasses.py

+                '{collection_req!s}: {error_messages!s}'.format(
+                    collection_req=collection_req,


Suggested change

'{collection_req!s}: {error_messages!s}'.format(

collection_req=collection_req,

f'{collection_req!s}: {{error_messages!s}}'.format(

webknjaz · 2025-03-24T18:06:48Z

test/units/cli/test_galaxy.py

-    expected += r"Also 'name' is not an FQCN\. A valid collection name must be in the format <namespace>\.<collection>\. "
-    expected += r"Please make sure that the namespace and the collection name contain characters from \[a\-zA\-Z0\-9_\] only\."
+def test_parse_requirements_contains_name_key(requirements_cli, requirements_file):
+    expected = "Failed to parse collection requirement entry: {'version': '1.0.0'}: missing required arguments: name"


The error message with a phrase separated with 3 colons and has another colon in a piece of structured data looks weird to me. It's probably worth rephrasing.

ansibot added bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. has_issue labels Aug 3, 2024

cavcrosby force-pushed the raise-on-invalid-collection-entry branch 2 times, most recently from 84ea136 to 8050c4d Compare August 3, 2024 22:19

cavcrosby marked this pull request as ready for review August 3, 2024 22:48

felixfontein reviewed Aug 6, 2024

View reviewed changes

cavcrosby force-pushed the raise-on-invalid-collection-entry branch from 8050c4d to 44f7d8e Compare August 6, 2024 13:04

mkrizek requested a review from s-hertel August 6, 2024 15:07

mkrizek removed the needs_triage Needs a first human triage before being processed. label Aug 6, 2024

felixfontein reviewed Aug 6, 2024

View reviewed changes

s-hertel reviewed Aug 6, 2024

View reviewed changes

lib/ansible/galaxy/dependency_resolution/dataclasses.py Outdated Show resolved Hide resolved

cavcrosby force-pushed the raise-on-invalid-collection-entry branch 4 times, most recently from 476d5b5 to 881b76f Compare August 9, 2024 17:36

cavcrosby commented Aug 9, 2024

View reviewed changes

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 21, 2024

webknjaz reviewed Nov 15, 2024

View reviewed changes

webknjaz requested review from jborean93 and s-hertel November 15, 2024 01:36

cavcrosby force-pushed the raise-on-invalid-collection-entry branch 2 times, most recently from 81a3f34 to d3f3a2f Compare January 19, 2025 17:46

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jan 19, 2025

cavcrosby force-pushed the raise-on-invalid-collection-entry branch from d3f3a2f to e418715 Compare January 19, 2025 19:00

ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jan 19, 2025

This comment was marked as resolved.

Sign in to view

cavcrosby force-pushed the raise-on-invalid-collection-entry branch from 3b1fa5a to 8b0dc8b Compare January 19, 2025 22:48

ansibot removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. ci_verified Changes made in this PR are causing tests to fail. labels Jan 19, 2025

cavcrosby force-pushed the raise-on-invalid-collection-entry branch from 8b0dc8b to 0f5fcea Compare January 20, 2025 15:17

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jan 20, 2025

This comment was marked as outdated.

Sign in to view

cavcrosby force-pushed the raise-on-invalid-collection-entry branch from 0f5fcea to 7bf8e24 Compare January 20, 2025 18:12

webknjaz removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jan 21, 2025

s-hertel reviewed Jan 24, 2025

View reviewed changes

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 30, 2025

webknjaz reviewed Jan 30, 2025

View reviewed changes

cavcrosby force-pushed the raise-on-invalid-collection-entry branch from 7bf8e24 to b378d12 Compare February 2, 2025 21:51

Don't ignore invalid collection entries in requirements.yml (ansible#…

38db67b

…83699)

cavcrosby force-pushed the raise-on-invalid-collection-entry branch from b378d12 to 38db67b Compare February 2, 2025 22:20

ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 2, 2025

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 14, 2025

ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Mar 3, 2025

webknjaz reviewed Mar 24, 2025

View reviewed changes

s-hertel mentioned this pull request Aug 20, 2025

Type-annotate ansible.galaxy.dependency_resolution #85545

Merged

	"The foo 'source' key is not applicable when 'name' is not a FQCN."
	r"The foo 'source' key is not applicable when 'name' is not a FQCN\."

	type=dict(type='str'),
	type=dict(type='str', choices=['file', 'galaxy', 'git', 'url', 'dir', 'subdirs']),

		Requirement.from_requirement_dict({'name': to_text(collection_artifact[1]), 'invalid_key': 'foo', 'key': 'bar'}, concrete_artifact_cm)


		def test_build_requirement_from_tar_invalid_name_with_source(collection_artifact):

	def test_build_requirement_from_tar_invalid_name_with_source(collection_artifact):
	def test_build_requirement_invalid_name_with_source(collection_artifact):

		@@ -0,0 +1,2 @@
		bugfixes:
		- ansible-galaxy collection install - raise errors on invalid collection entry in requirements.yml (https://github.com/ansible/ansible/issues/83699).

	- ansible-galaxy collection install - raise errors on invalid collection entry in requirements.yml (https://github.com/ansible/ansible/issues/83699).
	- ansible-galaxy collection install\|download\|verify - raise errors on invalid collection entry in requirements.yml (https://github.com/ansible/ansible/issues/83699).

	- ansible-galaxy collection install - raise errors on invalid collection entry in requirements.yml (https://github.com/ansible/ansible/issues/83699).
	- ``ansible-galaxy collection install\|download\|verify`` - raise errors on invalid collection entry in :file:`requirements.yml` (https://github.com/ansible/ansible/issues/83699).

		except AnsibleError as err:
		pytest.fail(f'Failed to handle a collection with a pre-release version: {err}')

	f"The {req_name} 'version' key is not a valid version."
	f"The {req_name} 'version' key is not a valid version: "
	f"'{segment}' part of '{req_version}' does not follow SemVer."

		@@ -0,0 +1,2 @@
		bugfixes:
		- "``ansible-galaxy collection install\|download\|verify`` - raise errors on invalid collection entry in `requirements.yml` (https://github.com/ansible/ansible/issues/83699)."

		'{collection_req!s}: {error_messages!s}'.format(
		collection_req=collection_req,

	'{collection_req!s}: {error_messages!s}'.format(
	collection_req=collection_req,
	f'{collection_req!s}: {{error_messages!s}}'.format(

Don't ignore invalid collection entries in requirements.yml #83715

Are you sure you want to change the base?

Don't ignore invalid collection entries in requirements.yml #83715

Conversation

cavcrosby commented Aug 3, 2024

SUMMARY

ISSUE TYPE

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cavcrosby Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cavcrosby commented Nov 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webknjaz Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-hertel Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as outdated.

s-hertel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

cavcrosby Aug 9, 2024 •

edited

Loading

webknjaz Jan 30, 2025 •

edited

Loading

s-hertel Jan 30, 2025 •

edited

Loading

cavcrosby Jan 25, 2025 •

edited

Loading