_dependency_source, _cli, test: Do not override `pip.conf` unless explicitly specified via flags #565

philblckwd · 2023-03-22T01:58:29Z

Refactor index-url and extra-index-url options to not override config defined in 'pip.conf' unless explicitly specified via flags.

Closes #563

woodruffw · 2023-03-22T15:49:37Z

pip_audit/_cli.py

@@ -286,7 +285,6 @@ def _parser() -> argparse.ArgumentParser:  # pragma: no cover
        type=str,
        help="base URL of the Python Package Index; this should point to a repository compliant "
        "with PEP 503 (the simple repository API)",
-        default=PYPI_URL,


I think we want to keep this default in the CLI -- behaviorally it should remain the default, and having it appear as such in the --help render makes for a nicer UX 🙂

So leaving this as the default means that it will always be explicitly passed to the pip install CLI, which results in the index-url specified in the pip.conf being ignored. I understand that it makes for a more friendly UX to specify as such, but would it be reasonable to convey that the url will be resolved by pip if not passed by the user explicitly?

If this is a deal breaker though, then I'll have to dig deeper into pip to determine how it is performing the index-url resolution.

So leaving this as the default means that it will always be explicitly passed to the pip install CLI, which results in the index-url specified in the pip.conf being ignored. I understand that it makes for a more friendly UX to specify as such, but would it be reasonable to convey that the url will be resolved by pip if not passed by the user explicitly?

That sounds reasonable to me!

woodruffw · 2023-03-22T15:50:55Z

pip_audit/_dependency_source/pyproject.py

-        self, filename: Path, index_urls: list[str] = [PYPI_URL], state: AuditState = AuditState()
+        self,
+        filename: Path,
+        index_url: str = "",


Similar typestate nit here: maybe drop the default "" here (or make it PYPI_URL), since we always expect a valid URL?

Sorry, this was a misunderstanding on my part! I think we want this to be:

Suggested change

index_url: str = "",

index_url: str | None = None,

Yep, I'd prefer this too.

woodruffw · 2023-03-22T15:51:09Z

pip_audit/_dependency_source/requirement.py

@@ -47,7 +46,8 @@ def __init__(
        require_hashes: bool = False,
        no_deps: bool = False,
        skip_editable: bool = False,
-        index_urls: list[str] = [PYPI_URL],
+        index_url: str = "",


Same as https://github.com/pypa/pip-audit/pull/565/files#r1145039056

Suggested change

index_url: str = "",

index_url: str | None = None,

woodruffw · 2023-03-22T15:53:04Z

pip_audit/_virtual_env.py

@@ -44,7 +43,8 @@ class VirtualEnv(venv.EnvBuilder):
    def __init__(
        self,
        install_args: list[str],
-        index_urls: list[str] = [PYPI_URL],
+        index_url: str = "",


Oh, this explains it! Rather than using an empty string as falsey, let's make this an optional type:

Suggested change

index_url: str = "",

index_url: str | None = None,

...since that'll better convey the valid states here.

woodruffw · 2023-03-22T15:54:57Z

Thanks a ton for opening this, @philblckwd!

This looks great to me structurally; just a few small nitpicks 🙂

woodruffw · 2023-03-22T15:55:35Z

(This also needs a CHANGELOG.md entry. Feel free to do it yourself if you feel like it, otherwise I'll do it as a fixup after merge!)

tetsuo-cpp · 2023-03-23T01:02:59Z

I'm also happy with where this is at. Once the help text is amended and those type state related comments are addressed, this should be good to go.

Thanks again for your work. We really appreciate it.

tetsuo-cpp · 2023-03-23T01:04:32Z

@philblckwd One more thing. We have a check in CI to ensure that the help text that we display in the README is up to date. So once you've amended the help text for --index-url, you'll need to update the README.

… defined in 'pip.conf' unless explicitly specified via flags.

philblckwd · 2023-03-24T01:48:57Z

Per feedback:

I updated arguments to use optional types
I updated the help text, as well as the readme, to state the index-url will be resolved by pip if not specified
I added an entry to the changelog
I noticed unrelated lint and check-readme errors, so resolved those too

This reverts commit 54d7ce9.

tetsuo-cpp

LGTM!

tetsuo-cpp · 2023-03-24T02:53:32Z

I'll merge this shortly after I've done a bit of testing locally.

tetsuo-cpp · 2023-03-24T06:18:16Z

Thanks @philblckwd!

## [2.5.4] ### Changed * Refactored `index-url` option to not override user pip config by default, unless specified ([#565](pypa/pip-audit#565)) ### Fixed * Fixed bug with the `--fix` flag where new requirements were sometimes being appended to requirement files instead of patching the existing requirement ([#577](pypa/pip-audit#577)) * Fixed a crash caused by auditing requirements files that refer to other requirements files ([#568](pypa/pip-audit#568)) ## [2.5.3] ### Changed * Further simplified `pip-audit`'s dependency resolution to remove inconsistent behaviour when using hashed requirements or the `--no-deps` flag ([#540](pypa/pip-audit#540)) ### Fixed * Fixed a crash caused by invalid UTF-8 sequences in subprocess outputs ([#572](pypa/pip-audit#572)) ## [2.5.2] ### Fixed * Fixed a loose dependency constraint for CycloneDX SBOM generation ([#558](pypa/pip-audit#558))

philblckwd mentioned this pull request Mar 22, 2023

Private repo dependencies no longer being skipped #563

Closed

tetsuo-cpp self-requested a review March 22, 2023 06:57

woodruffw reviewed Mar 22, 2023

View reviewed changes

woodruffw added enhancement New feature or request component:dep-sources Dependency sources labels Mar 22, 2023

Phil Blackwood added 6 commits March 23, 2023 17:02

Refactor index-url and extra-index-url options to not override config…

7e2cfa3

… defined in 'pip.conf' unless explicitly specified via flags.

Update index_url type to optional

db77e39

Update index_url help text

f68a573

Fix discrepancy between help text and README output

54d7ce9

Update changelog

b05bf32

Fix lint issue

e01b2f3

philblckwd force-pushed the feature/index-url-usage branch from 1308173 to e01b2f3 Compare March 24, 2023 01:42

Revert "Fix discrepancy between help text and README output"

4456d84

This reverts commit 54d7ce9.

tetsuo-cpp changed the title ~~feature/index-url-usage~~ _dependency_source, _cli, test: Do not override pip.conf unless explicitly specified via flags Mar 24, 2023

tetsuo-cpp approved these changes Mar 24, 2023

View reviewed changes

tetsuo-cpp merged commit 36973e9 into pypa:main Mar 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

_dependency_source, _cli, test: Do not override `pip.conf` unless explicitly specified via flags #565

_dependency_source, _cli, test: Do not override `pip.conf` unless explicitly specified via flags #565

philblckwd commented Mar 22, 2023 •

edited by tetsuo-cpp

Loading

woodruffw Mar 22, 2023

philblckwd Mar 22, 2023

tetsuo-cpp Mar 23, 2023

woodruffw Mar 22, 2023

woodruffw Mar 22, 2023

tetsuo-cpp Mar 23, 2023

woodruffw Mar 22, 2023

woodruffw Mar 22, 2023

woodruffw Mar 22, 2023

woodruffw commented Mar 22, 2023

woodruffw commented Mar 22, 2023 •

edited

Loading

tetsuo-cpp commented Mar 23, 2023

tetsuo-cpp commented Mar 23, 2023

philblckwd commented Mar 24, 2023

tetsuo-cpp left a comment

tetsuo-cpp commented Mar 24, 2023

tetsuo-cpp commented Mar 24, 2023

_dependency_source, _cli, test: Do not override pip.conf unless explicitly specified via flags #565

_dependency_source, _cli, test: Do not override pip.conf unless explicitly specified via flags #565

Conversation

philblckwd commented Mar 22, 2023 • edited by tetsuo-cpp Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodruffw commented Mar 22, 2023

woodruffw commented Mar 22, 2023 • edited Loading

tetsuo-cpp commented Mar 23, 2023

tetsuo-cpp commented Mar 23, 2023

philblckwd commented Mar 24, 2023

tetsuo-cpp left a comment

Choose a reason for hiding this comment

tetsuo-cpp commented Mar 24, 2023

tetsuo-cpp commented Mar 24, 2023

_dependency_source, _cli, test: Do not override `pip.conf` unless explicitly specified via flags #565

_dependency_source, _cli, test: Do not override `pip.conf` unless explicitly specified via flags #565

philblckwd commented Mar 22, 2023 •

edited by tetsuo-cpp

Loading

woodruffw commented Mar 22, 2023 •

edited

Loading