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

Always stringify collection requirement versions #81694

Draft
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

webknjaz
Copy link
Member

Before this change, whenever a numeric version was specified in requirements.yml, the YAML parser would read it as int or float which would leak straight down into the dependency resolver. This patch makes an unconditional conversion of that data into str right before constructing Requirement objects.

Fixes #79109
Fixes #78067

SUMMARY

$sbj.

ISSUE TYPE
  • Bugfix Pull Request
ADDITIONAL INFORMATION

N/A

Before this change, whenever a numeric version was specified in
`requirements.yml`, the YAML parser would read it as `int` or `float`
which would leak straight down into the dependency resolver. This
patch makes an unconditional conversion of that data into `str`
right before constructing `Requirement` objects.

Fixes ansible#79109
Fixes ansible#78067
@@ -310,6 +310,8 @@ def from_string(cls, collection_input, artifacts_manager, supplemental_signature
def from_requirement_dict(cls, collection_req, art_mgr, validate_signature_options=True):
req_name = collection_req.get('name', None)
req_version = collection_req.get('version', '*')
if req_version is not None:
req_version = str(req_version)
Copy link
Member Author

Choose a reason for hiding this comment

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

@s-hertel have you considered just doing this bare minimum?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's applying to all the potential matches in the provider, is it? Needs some tests, have no idea if/what issues this is fixing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I know about the tests. I was just pulling some things from my git stash and turning them into draft PRs so they don't rot there.
This is an attempt to address the case when requirements.yml has a version that is parsed by PyYAML as an int or a float causing TypeError tracebacks when some logic down the line assumes the ver attribute is always a string, calling str methods.

@ansibot ansibot added bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. has_issue labels Sep 13, 2023
@s-hertel s-hertel removed their request for review September 13, 2023 13:34
@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Sep 14, 2023
@ansibot 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 Sep 21, 2023
@ansibot ansibot added the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. label Dec 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.18 bug This issue/PR relates to a bug. has_issue needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested.
Projects
None yet
5 participants