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

Configure private PyPi repositories in the environment #471

Conversation

jacksmith15
Copy link
Contributor

@jacksmith15 jacksmith15 commented Jul 17, 2023

Description

Resolves #460

This PR adds support for configuring private PyPi repositories via environment variables. This avoids users needing to install an exact version of poetry to manage the configuration format, and then copy around credential files to make them discoverable to conda-lock. See #460 for full reasoning.

After this change, users are able to set the CONDA_LOCK_PYPI_REPOSITORY_<repository-name> environment variable to configure new PyPi repositories.

An alternative could be to support PIP_EXTRA_INDEX_URL instead, as this will be respected during conda-lock install, meaning only a single environment variable is required for both lock and install.

@jacksmith15 jacksmith15 requested a review from a team as a code owner July 17, 2023 16:27
@netlify
Copy link

netlify bot commented Jul 17, 2023

Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit 665e8c8
🔍 Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/64ff2091c5082e0008082e56
😎 Deploy Preview https://deploy-preview-471--conda-lock.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Comment on lines 377 to 379
factory.create_legacy_repository(
{"name": source[0], "url": source[1]["url"]}, config
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've left the previous method here so this is not a breaking change.

However since this is exposing an implementation detail as an interface, it might be worth adding a deprecation warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we're ready yet to deprecate it. I'm very eager to ditch anything Poetry-related, but setting up environment variables can be a pain, and until we get something better in place, some people may prefer the Poetry interface.

Copy link
Contributor

@maresb maresb left a comment

Choose a reason for hiding this comment

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

Looking very good. Let's keep poetry config repositories.foo in the docs, but put your method first.

Comment on lines 377 to 379
factory.create_legacy_repository(
{"name": source[0], "url": source[1]["url"]}, config
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we're ready yet to deprecate it. I'm very eager to ditch anything Poetry-related, but setting up environment variables can be a pain, and until we get something better in place, some people may prefer the Poetry interface.

Comment on lines +378 to +380
factory.create_legacy_repository({"name": name, "url": url}, config)
for name, url in _parse_repositories_from_environment().items()
] + [
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please write a test for _prepare_repositories_pool to exercise this functionality?

Comment on lines +165 to +177
Alternatively, you can use the `poetry` configuration file format to configure private PyPi repositories. The configuration file should be named `config.toml` and have the following format:

```toml
[repositories.example]
url = "https://username:[email protected]/simple"
```

The discoverable location of this file depends on the operating system:

- **Linux**: `$XDG_CONFIG_HOME/pypoetry-conda-lock` or `~/.config/pypoetry-conda-lock`
- **OSX**: `~/Library/Application Support/pypoetry-conda-lock`
- **Windows**: Check output of `python -c 'from conda_lock._vendor.poetry.locations import CONFIG_DIR;print(CONFIG_DIR)'`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maresb I have restored documentation for the existing repository configuration method here.

Since the previous documentation was incorrect, I have changed the documented process for configuring it this way. This is because the poetry config repositories.foo will write configuration to the wrong appdirs directory for conda-lock to discover (~/.config/pypoetry vs ~/.config/pypoetry-conda-lock).

I have also written it in a way which avoids users needing to install the exact version of poetry vendored by conda-lock.

Also note: I couldn't decipher which directory to recommend for windows, as I suspect its platform-dependent, so that's the best I could do. The old documentation didn't have this problem for the same reason it didn't work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks a lot for catching this! Seems like we broke this in #342.

@jacksmith15
Copy link
Contributor Author

@maresb thanks for the review, and apologies for the delay in updating.

I think I've addressed all of your previous feedback now 👍

@maresb
Copy link
Contributor

maresb commented Jul 25, 2023

Hey @jacksmith15, I feel really bad for doing this, but after taking a fresh look I think we are on the wrong track here. 😢 I should have caught this earlier in the review process, my bad.

I think a much better approach would be a parallel implementation of authenticated channels where the corresponding documentation would be essentially identical, but substituting channelspypi-repositories.

Do you agree? If so, would you have any time to rework this into a (partial) implementation?

I'm terribly sorry about sending you down the wrong track. Please let me know if I can do anything to help.

@jacksmith15
Copy link
Contributor Author

jacksmith15 commented Jul 25, 2023

To be clear, the suggested approach would involve adding a new non-standard key to the source environment.yml, e.g.

channels:
  - https://host.tld/t/$QUETZ_API_KEY/channel_name
  - conda-forge
repositories:
  - https://$MY_USER:[email protected]/pypi/simple

Does that look right to you? I note this doesn't support naming repositories, which is how they are generally supplied to the poetry solver. Should they be auto-named, or break the structural convention in the file?

Should this also provide a mechanism for enabling/disabling PyPi? Similar to nodefaults in channels?

Additionally, should --strip-auth apply to env var references?

@maresb
Copy link
Contributor

maresb commented Jul 25, 2023

These are very good questions. You are understanding my proposal correctly.

We already have a precedent for defining a non-standard key in environment.yml, namely platforms, so I think adding another should be okay.

I personally would prefer to be more specific with the key name and have it be pypi-repositories just because "repository" is such a general term, and I get confused wondering what's Conda-related and what's pip-related. Maybe it should be called pip-repositories? (Note - characters should be fine for key names in yaml, toml, and command-line arguments.)

I think it's important to avoid Poetry's influence which may push us towards bad design decisions, so I'd prefer autonaming to having a discrepancy between channels and repositories. Do you foresee any problems that might arise with this approach?

A mechanism for disabling PyPI outside of pyproject.toml and more in line with this system would be interesting, but certainly not necessary at this stage. Probably it would be best to defer this to another PR.

Supporting --strip-auth would be nice, but if it's a lot of work I think it'd be fine to instead add a prominent disclaimer.

@jacksmith15
Copy link
Contributor Author

jacksmith15 commented Aug 1, 2023

@maresb regarding the naming, its actually a little tricky to give a good name for Python repositories. I think the most "correct" term is "python simple repository": https://packaging.python.org/en/latest/specifications/simple-repository-api/#simple-repository-api

  • pypi refers specifically to https://pypi.org/
  • pip refers to the package manager, which is one way to interact with Python Package repositories which conform to the Simple API

However simple-repositories has a similar issue of being quite vague! I suppose one could do pep-503-repositories but that requires the reader to have additional context.

I'll prefer pip-repositories for now as that's probably most familiar to most people, but worth considering which you prefer.

@maresb
Copy link
Contributor

maresb commented Aug 1, 2023

@jacksmith15, I agree that I think pip-repositories is the best choice.

@jacksmith15
Copy link
Contributor Author

📝 I haven't forgotten about this!

I am working on the alternative solution in #481

There's some differences with pip repositories compared to channels that need handling, but I the general direction seems 👍

@maresb maresb marked this pull request as draft August 30, 2023 02:23
@maresb
Copy link
Contributor

maresb commented Oct 17, 2023

Superseded by #529

@maresb maresb closed this Oct 17, 2023
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.

Improve auth configuration for private PyPi repositories
2 participants