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

Use env vars if either --username or --password (or both) is missing #5584

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

Conversation

dszakallas
Copy link

@dszakallas dszakallas commented May 9, 2022

Pull Request Check List

Resolves: #5526

  • Added tests for changed code.
  • Updated documentation for changed code.

This PR changes authentication logic for publishing:

  • If both username and password is missing, consider PyPI token env var.
  • If no PyPI token and either username or password is missing, consider http basic env var for the missing field.

Previously, env vars were only considered if both CLI options where missing. This PR effectively makes it possible to mix the CLI options with the env vars.

This way both

poetry config repositories.myrepo $REPO_URL \
  && POETRY_HTTP_BASIC_MYREPO_PASSWORD=$PASSWORD \
     poetry publish -r myrepo -u $USERNAME

poetry config repositories.myrepo $REPO_URL \
  && POETRY_HTTP_BASIC_MYREPO_USERNAME=$USERNAME \
     poetry publish -r myrepo -p $PASSWORD

will work. I am doubtful about whether it is worth supporting the second use case.

# Check if we have a token first
token = self._authenticator.get_pypi_token(repository_name)
token = None
if not (username or password):
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is desired.

Copy link
Author

Choose a reason for hiding this comment

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

The original behavior would overwrite any value passed via CLI, if either is missing and a token is found (e.g if a username is passed and a token is found, then it would discard the username). Do we want to consider token auth if there's a username? I am happy to change back to the original behavior, although I find it a bit unintuitive.

username = auth.username
password = auth.password
username = username or auth.username
password = password or auth.password
Copy link
Member

Choose a reason for hiding this comment

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

Unsure if we really have this scenario.

Copy link
Author

Choose a reason for hiding this comment

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

This looks very niche to me as well. Do you think we should not consider this case?

@neersighted
Copy link
Member

I'm still not sure on this myself, but I would like to see tests (plus dropping the second case) to consider this for merging.

@abn abn force-pushed the publishing-env-vars branch from 53d8d4c to 5d00496 Compare January 17, 2025 20:06
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.

Mixing POETRY_HTTP_BASIC_ env vars with CLI args does not work on poetry publish
3 participants