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

Refactor MultiDomainBasicAuth #12934

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

Conversation

jfly
Copy link
Contributor

@jfly jfly commented Aug 24, 2024

(This PR is some refactoring that I want to land before creating a PR to implement #12750. If the additional context is useful, you can see my upcoming changes over in jfly/pip@jfly:issue-12750-pre-work-refactor...jfly:pip:issue-12750-configurable-keyring-subprocess-location)

I was starting to work on #12750, but
got hung up on some strange OOP patterns going on here. In particular, I
found it odd that there are 2 different ways we set some knobs
(prompting and keyring_provider) and for instances of
MultiDomainBasicAuth:

  1. In tests, we do it by passing these knobs as constructor
    parameters to MultiDomainBasicAuth.
  2. In the real cli codepath, we do it by mutating session.auth (an
    instance of MultiDomainBasicAuth) after it's constructed.

I believe that 2) is the reason for the careful cache management logic
you see me tearing out in the PR. Basically: when a
MultiDomainBasicAuth is constructed, it's possible that the keyring
provider will change after the fact, so we can't do any useful caching
in our constructor.

In this PR, I reworked all of this to behave like more normal OOP: I've
tightened up the API of MultiDomainBasicAuth so that these knobs are
only passed in as constructor parameters, and the other instance
attributes are now "private" (underscore-prefixed) values that you're
not supposed to read or write to (except for some tests that look at
these attributes to verify things are working).

This change allowed me to get rid of all the careful cache management
code we had, and sets me up to implement
#12750 as I'm going to be adding yet
another knob (keyring_executable). This new pattern will allow me to
validate that keyring_executable is compatible with keyring_provider
in MultiDomainAuthSettings's constructor (it doesn't make any sense to
specify a path to an executable if you're using the import provider, for
example). With the previous code
pattern, there was just no good place to validate that.

You'll notice I did end up touching a couple of tests:

  • test_collector.py: Logging is slightly different now, as we now
    immediately go fetch the keyring provider, rather than lazily. I could
    rework this to preserve the old behavior, but I don't think it's an
    important difference, it only affects tests that are trying to assert
    on every single log message.
  • test_network_auth.py: Nothing significant here. Just removing the
    now-unnecessary reset_keyring logic, and updating .password to
    ._password to reflect the new api.

@jfly jfly force-pushed the issue-12750-pre-work-refactor branch from 34e2385 to b23d14f Compare August 24, 2024 07:19
I found the original code a little dense. IMO, this refactor makes it a
little easier to see what's going on, and more importantly, *why*.
@jfly jfly force-pushed the issue-12750-pre-work-refactor branch 2 times, most recently from a19d48d to 082a173 Compare August 24, 2024 08:07
I was starting to work on pypa#12750, but
got hung up on some strange OOP patterns going on here. In particular, I
found it odd that there are 2 different ways we set some knobs
(`prompting` and `keyring_provider`) and for instances of
`MultiDomainBasicAuth`:

1. In tests, we do it by passing these knobs as constructor
   parameters to `MultiDomainBasicAuth`.
2. In the real cli codepath, we do it by mutating `session.auth` (an
   instance of `MultiDomainBasicAuth`) after it's constructed.

I believe that 2) is the reason for the careful cache management logic
you see me tearing out in the PR. Basically: when a
`MultiDomainBasicAuth` is constructed, it's possible that the keyring
provider will change after the fact, so we can't do any useful caching
in our constructor.

In this PR, I reworked all of this to behave like more normal OOP: I've
tightened up the API of `MultiDomainBasicAuth` so that these knobs are
only passed in as constructor parameters, and the other instance
attributes are now "private" (underscore-prefixed) values that you're
not supposed to read or write to (except for some tests that look at
these attributes to verify things are working).

This change allowed me to get rid of all the careful cache management
code we had, and sets me up to implement
pypa#12750 as I'm going to be adding yet
another knob (`keyring_executable`). This new pattern will allow me to
validate that `keyring_executable` is compatible with `keyring_provider`
in `MultiDomainAuthSettings`'s constructor (it doesn't make any sense to
specify a path to an executable if you're using the import provider, for
example). With the previous code
pattern, there was just no good place to validate that.

You'll notice I did end up touching a couple of tests:

- `test_collector.py`: Logging is slightly different now, as we now
  immediately go fetch the keyring provider, rather than lazily. I could
  rework this to preserve the old behavior, but I don't think it's an
  important difference, it only affects tests that are trying to assert
  on every single log message.
- `test_network_auth.py`: Nothing significant here. Just removing the
  now-unnecessary `reset_keyring` logic, and updating `.password` to
  `._password` to reflect the new api.
@jfly jfly force-pushed the issue-12750-pre-work-refactor branch from 082a173 to 1ec2c8d Compare August 24, 2024 08:08
@jfly jfly marked this pull request as ready for review August 24, 2024 08:25
@jfly
Copy link
Contributor Author

jfly commented Aug 24, 2024

@Darsstar @uranusjr I'd appreciate your thoughts on this PR!

Copy link
Contributor

@Darsstar Darsstar left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

3 participants