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

Reading credentials from .pypirc #789

Merged
merged 15 commits into from
Jan 13, 2024
Merged

Reading credentials from .pypirc #789

merged 15 commits into from
Jan 13, 2024

Conversation

funkyfuture
Copy link
Contributor

this is an approach to solve #561.

another way could be to wrap it all up in one credentials object that contains all the logic, including the cache updates.

therefore i'd appreciate feedback in this regard.

@ofek
Copy link
Sponsor Collaborator

ofek commented Nov 27, 2023

Any desire to rebase and pick this back up?

@funkyfuture
Copy link
Contributor Author

yes, likely within some weeks.

looking at the proposal again, personally i'd really prefer a complete Authentication class, i put some comments in where that'd matter. please let me know what you prefer.

@funkyfuture
Copy link
Contributor Author

okay, i rebased and completed the feature.

@funkyfuture
Copy link
Contributor Author

@ofek i don't know the interfaces here good enough, excuse me if this is redundant: i amended changes and the CI needs to be kicked off again.

@funkyfuture
Copy link
Contributor Author

@ofek i rebased again. can you kick off the checks again? force-pushing seems to not play along well in the system.

@funkyfuture
Copy link
Contributor Author

happy new year @ofek, i'd appreciate a feedback here.

@ofek
Copy link
Sponsor Collaborator

ofek commented Jan 2, 2024

Happy new year to you as well! I plan to include this in the next minor release it's just that there are a lot of changes here so it takes up a lot more time to review.

Would you be willing to refactor a bit less so I can review this faster? If not it's okay and I just have to schedule extra free time

@funkyfuture
Copy link
Contributor Author

Would you be willing to refactor a bit less so I can review this faster?

not really without specific suggestions. and i also think that the refactoring is worth it as it consolidates how the different credentials sources are considered (i could argue that this is overdue and thus should be prioritized over new features) and thus should allow easier extensions in the future.

src/hatch/utils/auth.py Outdated Show resolved Hide resolved
src/hatch/utils/auth.py Outdated Show resolved Hide resolved
tests/cli/publish/test_publish.py Outdated Show resolved Hide resolved
docs/publish.md Outdated Show resolved Hide resolved
src/hatch/publish/index.py Outdated Show resolved Hide resolved
@funkyfuture funkyfuture requested a review from ofek January 2, 2024 21:25
@funkyfuture
Copy link
Contributor Author

thanks for the valuable feedback.

@ofek
Copy link
Sponsor Collaborator

ofek commented Jan 2, 2024

Note this resolves #1191

Copy link
Sponsor Collaborator

@ofek ofek left a comment

Choose a reason for hiding this comment

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

Almost done, thanks!

mkdocs.yml Outdated Show resolved Hide resolved
docs/how-to/publishing/authentication.md Outdated Show resolved Hide resolved
docs/how-to/publishing/authentication.md Outdated Show resolved Hide resolved
docs/how-to/publishing/authentication.md Outdated Show resolved Hide resolved
docs/how-to/publishing/authentication.md Outdated Show resolved Hide resolved
docs/how-to/publishing/authentication.md Outdated Show resolved Hide resolved
docs/publish.md Outdated Show resolved Hide resolved
@funkyfuture
Copy link
Contributor Author

a'ight then.

@funkyfuture funkyfuture requested a review from ofek January 3, 2024 17:59
Copy link
Sponsor Collaborator

@ofek ofek left a comment

Choose a reason for hiding this comment

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

Thank you very much for your efforts and your patience!

@ofek ofek merged commit f59508b into pypa:master Jan 13, 2024
38 checks passed
github-actions bot pushed a commit that referenced this pull request Jan 13, 2024
* publish: Moves credential obtaining to a utils module

* publish: Implements credentials retrieval from .pypirc

* publish: Moves PyPI credential handling to one class

* cli/…/test_publish: Reformats with rush

* publish: Adds test for .pypirc retrieval

* publish: Use __token__ as username fallback on empty user input

* publish: Updates docs

* Deals with ruff's suggestions

* Explicit code formatting with ruff

* Moves the auth module into the publish package

* Undoes style changes in a test module

* Re-arranges docs about publishing

* index-publisher: Lazily imports AuthenticationCredentials

* publish docs: Various minor improvements & changes

Co-authored-by: Ofek Lev <ofekmeister@gmail.com>

* Renames publishing related How-To docs

---------

Co-authored-by: Ofek Lev <ofekmeister@gmail.com> f59508b
@lfvjimisola
Copy link

From what version of hatch is this expected to be available?

@ofek
Copy link
Sponsor Collaborator

ofek commented Mar 19, 2024

The next minor release.

@albertas
Copy link

albertas commented Apr 1, 2024

I see that hatch 1.9.4 version was released last week, but this feature was not yet included. Should we expect it to ship in 1.9.5?

@ofek
Copy link
Sponsor Collaborator

ofek commented Apr 1, 2024

This project uses semantic versioning so features only happen in minor releases e.g. 1.10.0

@vaab
Copy link

vaab commented Apr 4, 2024

I'm using last master, checked to have the commit f59508b, and actually am using pdb inside publish/auth.py to try to figure why the default file coming from the pypi 2FA token generation page is not accepted and read as-is.

It seems it looks for a main section, and not pypi, then looks in all sections and looks for a repository option to match with https://upload.pypi.org/legacy/. Neither of these strategy will succeed with the format proposed by pypi itself when asking for a non-specific all power token, which is:

[pypi]
  username = __token__
  password = xxxx

image
(Don't worry for this token, as it was deleted before publishing it here)

Did I miss something obvious, or is that made on purpose ? If yes, may I ask the rationale (or a pointer to it) ?
Workarounds are obvious for me, knowing the code, but this might raise questions from other users also.

@funkyfuture
Copy link
Contributor Author

@vaab not sure whether i'm missing something from your description, but it seems that the behaviour that you expect is tested in the commit you reference. please verify that this is the case.

@vaab
Copy link

vaab commented Apr 7, 2024

Sorry if I was not clear. I guess you are referring to this test.

However, the AuthenticationCredentials class in the test is instantiated in a specific way (setting repo as the empty string, which will get then defaulted to 'pypi' in this logic, and, as a consequence, will work). However that doesn't seem to reflect how it is instantiated in a normal run, where the repository name is defaulted to 'main' (instead of empty string) prior to the instantiation (cf: here)

In my case, with a very default/simple installation, without customization, the first part of the logic looks for a main section (and fails to find it), then switch to the second part of the logic, looking for every sections that would have a repository option matching a precise url (with ending slash). Without a context, the pertinence of this last equality check is difficult to grasp and all this seems brittle.

And this explains why the test works, and a real world example fails. I believe my setup is a very default setup, and its quite clear why it fails, and it comes from this default 'main' section lookup, and these 2 different logic. The test should test on 'main' string and not empty string if it wants to reflect what I feel is normal usage. I'm obviously missing a lot of context, so I'm sorry if there are obvious things I missed.

The workaround for me was to add an option repository in the pypi section:

[pypi]
  username = __token__
  password = xxxx
  repository = https://upload.pypi.org/legacy/

But this won't be expected by most user I guess (and the URL should be exactly matched, with leading /).

@vaab
Copy link

vaab commented Apr 21, 2024

As I have good reasons to think that this will be reported as an issue soon, should I open a bug request and or propose a PR to solve this ? I didn't get any perspective on this point. Many thanks for any feedback.

@ofek
Copy link
Sponsor Collaborator

ofek commented Apr 21, 2024

Yes please!

@funkyfuture
Copy link
Contributor Author

funkyfuture commented Apr 22, 2024 via email

@manmartgarc
Copy link

Is there a way to pass an option to hatch publish to pick a particular item from .pypirc? For example, I'd like to publish to test.pypi and I have an extra entry on my .pypirc flie with the token for the test domain; can I guide hatch publish to use that option?

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

7 participants