-
-
Notifications
You must be signed in to change notification settings - Fork 471
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
Fall back to Twine's keyring support in publish command #759
Conversation
c1e6df0
to
7c8c802
Compare
For posterity, Konsti suggested to use |
FYI: ~/Code/rye/target/debug/rye publish --repository rye-development-repo --repository-url https://europe-west4-python.pkg.dev/personal-projects-393014/rye-development-repo/ --username _json_key_base64 --token <KEY> dist/* -y So instead of letting twine read the Just one thought. In this case you pass the I wish I would be able to help out more, but I have learn more Rust. Might be something I can do on the side, if no one else takes this on. Thanks a lot @cnpryer for you help! |
Thanks for checking it out! If there's interest in this I'd like to:
Ideally you can do this when it's done and it'll hand off the repository url + username inputs to Twine.
And if you decide to save the username and repo url in Rye's credentials file, then if no token is resolved next time it'd just hand off those same inputs again. If I can I'll add tests for the credentials resolution. Something like first CLI, then ENV, then Rye or Twine. |
To be honest I think the best way to leave this would be to specifically only dish out for Twine's keyring support and then do something more robust. I'm not really in love with this, but if there's a way to quickly fix this issue for you I'm happy to add to this. |
Maybe also worth considering ditching |
If that's where you want to go with it. I do like the token experience we're going for, but if there's a way to easily incorporate another common and secure method maybe we can start with this? I was thinking at the least:
That way to start at least we're supporting the current token process, CLI-override, and keyring via Twine. I'll clean up what's going on in this PR, mark this ready for a review, and then we can figure out what makes sense. |
I think this is a good start. Is there any chance you could add basic tests for this? Maybe by mocking up a dummy endpoint. |
Yea I looked at Twine's tests out of the same interest. I also looked around uv's. I'll try to look at that more today if I can. For now I'll mark this as ready in case you wanted to play around with it. Also happy to set up a simple test for |
Would be great to add a basic test in. This is the type of code that is easy to break. |
I tried to crank that out real quick. I didn't finish. I'll come back to it again later. Converting to draft until then. |
4d789ac
to
a5c03c0
Compare
// write the credentials file | ||
let credentials_file = home.join("credentials"); | ||
if !credentials_file.is_file() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we write the home contents how are we managing changes to those contents? I had to remove the file for this to run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Today the home folder is shared so there is really no way to fuck around in it between tests. One option would be to restructure the code in common/mod.rs
to make two homes. One like used by all (concurrent), another one that uses the file lock to ensure only ever one test can use it at the time.
This comment was marked as resolved.
This comment was marked as resolved.
2b102c7
to
f74f610
Compare
5602d3f
to
c5847ac
Compare
beede8c
to
73946d9
Compare
We don't want to default to 'pypi'. Keyring backends can be used without this data. Keyring is set up with repo url and username. `--skip-save-credentials` is added to avoid unwanted credentials file usage in environments where keyring might be used directly.
We want to refactor the first step in the resolution logic to find an entry for the provided repository if it exists. If we are given a repository but an entry doesn't exist we can create the entry for the rest of the logic to flow from.
This refactor allows for both credentials and repository data to be resolved independent from eachother. This makes it easier to implement tricky logic that flows from various use-cases.
This refactors the section that handles prompting the user. To avoid tangled logic here, and in other parts of the code, we want to first establish if encryption or decryption should happen. This influences the prompts needed as well as give us the ability to move where these operations end up ocurring -- which cleans this up nicely. Our functions now return `None` if the user decides not to provide an input.
Depending on the data that has been already resolved, we may be able to default certain configuration. For example, if we have enough information for keyring backends we can resolve our default username. However, we don't want to resolve our default username in all use-cases.
If the user doesn't skip the save step, and if we have enough/valid data to save, then we save to the credentials file and encrypt our token if encryption is needed. Encryption and decryption functions are cleaned up for an easier read.
Username and repository url are needed for keyring. So we provide those and the rest of its args dynamically to pass the remaining resolution to Twine. We use `--non-interactive` to avoid invoking Twine prompts (we handle this prompting ourselves).
73946d9
to
7476dc1
Compare
@mitsuhiko At this stage the PR should be ready. I'll keep an eye out for any reported issues. One comment though is what I mentioned here.
If we are still looking to prioritize Rye's token management then I'd say let's just see if users provide feedback to prioritize keyring. I'll add some documentation on this. You can allow Rye to perform the fallback if you pass Expand the details below if you'd like to see all the manual testing I just did. Last example uses the encrypted token. Encrypt/decrypt prompts aren't persisted in the output. The "keychain" error is from exiting my keyring prompt.
I really want to ensure is that we are reliably providing encryption if the user opts into it. I did some extra testing around this, but I'll do some more over the next couple of days. |
We no longer default to 'pypi' for this arg
Any chance that Twine's |
Feel free to mention that here |
@charliermarsh can I do anything to help make this easier to review and consider? I have time this weekend to work on this. The majority of this change is a refactor of the publish command's code. Would you prefer I undo the keyring support to start with just the refactor? I understand some logic might be upstreamed to |
Hey @cnpryer, would you still be interested in merging this? Would be useful for me too! |
Hi! I'd be happy to help out if there's interest. |
Great! I'd really like to have rye support keyrings, so put together this less ambitious PR: But I think it's worth reopening this and pinging @mitsuhiko and @charliermarsh to see how they want to proceed |
From #741, Rye is unable to publish packages with Twine if users rely on keyring.
I'm not sure if the plan is to implement more of the publish logic ourselves. In case it's not or we're still far off from that, this PR offers a fix for #741 by updating the publish command logic to fall back to Twine's keyring support.
Changes:
--username
,--token
, and--repository-url
to allow keyring fallback.--skip-save-credentials
to prevent potential confusing behavior and provide users with an opt-out method.Testing:
Manual testing:
Edit: Cleaned up the description for latest changes.