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

feat(manager/pip_requirements): allow = as extra-url separator #29727

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Finkregh
Copy link

@Finkregh Finkregh commented Jun 17, 2024

See also: #29726

Changes

manager/pip_requirements

Before:

only --extra-index-url http... matched

After:

also --extra-index-url=http... matches

Context

These lines reflect CLI arguments, normal or expected behaviour is that --foo bar and --foo=bar lead to the same result.

This is not explicitly documented, but inferred from behaviour of many other tools: https://pip.pypa.io/en/stable/reference/requirements-file-format/

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

Please comment if the docs should be updated, and where.

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@CLAassistant
Copy link

CLAassistant commented Jun 17, 2024

CLA assistant check
All committers have signed the CLA.

@Finkregh Finkregh marked this pull request as ready for review June 17, 2024 14:07
@Finkregh
Copy link
Author

Finkregh commented Jun 17, 2024

The CI seems to be stuck :( @rarkins

@rarkins
Copy link
Collaborator

rarkins commented Jun 17, 2024

GitHub requires manual approval of workflows any time a new contributor pushes to their branch. They're not stuck as in bug, but stuck as in security

@Finkregh
Copy link
Author

I don't know how to fix the issues the CI is showing 🙈

@rarkins
Copy link
Collaborator

rarkins commented Jun 18, 2024

Focus on coverage first

Comment on lines +38 to +39
if (line.includes('=')) {
registryUrls = [line.split('=')[1]];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Due to split on = the flags will be included in the registryUrl too
ie. for

--index-url=https://artifactory.company.com/artifactory/api/pypi/python/simple --trusted-host artifactory.company.com --default-timeout 600

registryUrl will be:
https://artifactory.company.com/artifactory/api/pypi/python/simple --trusted-host artifactory.company.com --default-timeout 600

whereas we want:
https://artifactory.company.com/artifactory/api/pypi/python/simple

This is also the cause of the failing tests in CI.

Copy link
Author

@Finkregh Finkregh Jun 25, 2024

Choose a reason for hiding this comment

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

Ah, good point. I'll look into it as I find time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants