Add basic publish command #86

cnpryer · 2023-05-01T19:34:03Z

Closes #85

How it works:

chrispryer@Chriss-MacBook-Pro ~/g/s/rye-publish> rye publish --repository-url https://test.pypi.org/legacy/
Enter a passphrase (optional):

Require access token
Check args (--token)
Check ~/.rye/credentials
Prompt user ("go to https://upload.pypi.org/legacy/")
Offer encryption/decryption via passphrase
Store in ~/.rye/credentials encoded keyed by --repository (defaults "pypi")
Publish targets via twine (--repository-url)

See #86 (comment)

Summary of changes:

Added publish command
- Added twine to bootstrapping
- Added age for encrypt/decrypt with passphrases
- Added hex for encode/decode token data
- Added dialoguer for passphrase prompt

TODO:

Add command with basic requirements from twine
Allow pattern and multiple targets for dist positional arguments
Basic token handling
Hidden input
Encode/decode
Successful https://test.pypi.org/ publish
Clean up
Validated encryption & decryption via passphrase and ~/.rye/credentials

Considerations

Supporting pypirc in the first-pass
ring is smaller but age is easier to reason about being new to this and keyring looks nice (see comment tagged below)
Should rye bootstrap with an empty ~/.rye/credentials?
Adding a --no-passphrase / --no-interaction flag
OIDC support
Using rye with env vars

See #86 (comment)

rye/src/cli/publish.rs

Co-authored-by: Chris Pryer <14341145+cnpryer@users.noreply.github.com>

mitsuhiko · 2023-05-02T08:00:49Z

I think it's reasonable to use twine behind the scenes but I wonder if it wouldn't be better to explicitly handle authentication. The way twine currently works makes it quite user unfriendly to get to the right experience (eg: upload with tokens).

cnpryer · 2023-05-02T12:09:29Z

Love it. How does this sound?

rye publish
"No access token found for <url>\nAccess token: <wait for user input>"

IIRC that's roughly how cargo handles it.

mitsuhiko · 2023-05-02T12:58:24Z

I think what makes most sense is to propose something like this:

rye publish if it does not have a token on file, prints a message for the user to go to https://pypi.org/manage/account/token/ and create a global token
It offers to encrypt the token with a passphrase
It stores the token in ~/.rye/credentails keyed by repository
Whenever it needs a token, it feeds that token directly to twine, optionally decrypting it with the provided or prompted passphrase

Since it's not possible to restrict a token to a not yet created project, we probably want to create global tokens all the way but it would be possible to only ever store a restricted token by using pypitoken.

cnpryer · 2023-05-02T13:05:08Z

Sounds like a plan. Just a heads up I'll be pretty busy until Thursday. If I can't pick this back up before then I'll tackle this into the weekend.

TODO(cnpryer): Finish experimenting

Merge imports

TODO(cnpryer): Could probably still support .pypirc

cnpryer · 2023-05-03T23:13:27Z

Just want to tag f4ecb2e with https://packaging.python.org/en/latest/specifications/pypirc/

cnpryer · 2023-05-09T16:28:37Z

I'll mark this as ready to review. Struggling to find time, so maybe I can get some feedback for the next window I have to work on this. It might make sense to consider what maturin does. I've added a broader "considerations" list to the issue's description.

Edit: I’ll resolve the conflicts and probably push a few more changes to make this a little more robust. I can do this later, then I’ll mark as ready for review.

mitsuhiko · 2023-05-09T17:58:11Z

I will review this today!

cnpryer · 2023-05-09T18:30:42Z

I will review this today!

Thanks! I want to put a little more time into to the url cli argument (eg: https://test.pypi.org/legacy will fail with a redirect due to the missing trailing /) and a couple other house cleaning items, but otherwise it should be structurally ready for some feedback.

cnpryer · 2023-05-09T21:36:34Z

rye/src/cli/publish.rs

+    let token = if let Some(token) = cmd.token {
+        let secret = Secret::new(token);
+        let maybe_encrypted = prompt_maybe_encrypt(&secret)?;
+        let encoded = hex::encode(maybe_encrypted.expose_secret());


Could just encode the encrypted bytes. If its not encrypted we could just store it as-is.

Added in abd82b2

cnpryer

I'm thinking I could provide some of my thoughts to give you a kind of warm-start to help with this review.

I think the Url change is sensible enough for me to go and make. I'll try it out and if it works I'll push. (done: ba19eff)
I don't like always encoding/decoding the token. It's an easy fix and probably sensible to make, so I'll add that to my list. (done: abd82b2)
I don't like pad_hex+escape_string use, so I might add this to my list. At least to better understand the serialization/deserialization of the data.
You apparently own dialoguer which maturin uses. I'm thinking about adding this to my list and swapping rpassword out for it (used for passphrase prompt; done: b86d858).

Otherwise here are some questions we can focus on:

Should we bootstrap with a ~/.rye/credentials file? Currently this PR creates it if it doesn't exist upon request.
Should we add a "package-repository" table to the credentials file? Right now it's just

[pypi]
token = "your token"

So maybe something like

[package-repository]
pypi = {token = "your token"}

Should we add support for pypirc? And if so, do we want to always prioritize this file over any credentials data? We can just pass this immediately to twine if it's provided.
Do we want to support OIDC right away or get this first-pass merged and follow up?
Could we add some kind of interaction-skipping argument (something like --no-passphrase)?
Should we add environment variable support for the token? If so, RYE_REPOSITORY_URL_TOKEN or something?

cnpryer · 2023-05-10T01:12:52Z

rye/src/cli/publish.rs

+    #[arg(long, default_value = "https://upload.pypi.org/legacy/")]
+    repository_url: String,


Suggested change

#[arg(long, default_value = "https://upload.pypi.org/legacy/")]

repository_url: String,

#[arg(long, default_value = "https://upload.pypi.org/legacy/")]

repository_url: Url,

This could probably work. I'll check this out tonight.

This seems to work.

I made a comment about handling trailing slashes in the url. If twine doesn't handle this upfront for the user maybe we don't either. Here's the output the user would see

Enter a passphrase (optional): Uploading distributions to https://test.pypi.org/legacy Uploading rye_publish-0.1.0-py3-none-any.whl 100% ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 3.8/3.8 kB • 00:00 • ? ERROR RedirectDetected: https://test.pypi.org/legacy attempted to redirect to https://test.pypi.org/legacy/. Your repository URL is missing a trailing slash. Please add it and try again.

Added Url usage in ba19eff

cnpryer · 2023-05-10T01:26:22Z

rye/src/cli/publish.rs

+        .get(repository)
+        .and_then(|table| table.get("token"))
+        .map(|token| token.to_string())
+        .map(clean_hex)


clean_hex is hacky, so if you want me to spend more time here I'm happy to. This should also move if we do https://github.com/mitsuhiko/rye/pull/86/files#r1189156856 (conditional encode/decode)

Split into pad_hex and escape_string but I'd still want to better understand this. I think I understand the hex padding, but having to escape the string is confusing me.

cnpryer · 2023-05-10T01:26:36Z

rye/src/cli/publish.rs

+        .map(|token| token.to_string())
+        .map(clean_hex)
+    {
+        let decoded = hex::decode(token)?;


(conditional decode) https://github.com/mitsuhiko/rye/pull/86/files#r1189156856

Added in abd82b2

cnpryer · 2023-05-10T01:33:12Z

rye/src/cli/publish.rs

+        let secret = Secret::new(token);
+        let maybe_encrypted = prompt_maybe_encrypt(&secret)?;
+        credentials[repository]["token"] =
+            Item::Value(hex::encode(maybe_encrypted.expose_secret()).into());


(conditional encode) https://github.com/mitsuhiko/rye/pull/86/files#r1189156856

Added in abd82b2

cnpryer · 2023-05-12T19:20:15Z

I have time today and tomorrow morning to work on any changes here if needed.

mitsuhiko · 2023-05-12T20:56:34Z

rye/src/bootstrap.rs

@@ -46,6 +46,7 @@ platformdirs==3.4.0
 pyproject_hooks==1.0.0
 requests==2.29.0
 tomli==2.0.1
+twine==4.0.2


Since this adds a new dependency the SELF_VERSION needs bumping.

Added in e828154

mitsuhiko · 2023-05-12T20:57:43Z

rye/src/cli/publish.rs

+        secret
+    };
+
+    let mut publish_cmd = Command::new(venv.join("bin/python"));


This should use the new utility function that makes this work on windows (get_venv_python_bin).

Added in f6dc079

Oof I can fix the commit message if you'd like

rye/src/cli/publish.rs

mitsuhiko · 2023-05-12T21:44:56Z

Looks good!

cnpryer added 3 commits May 1, 2023 15:30

Add basic publish command

847cdc0

Fix docs

7a7b6f1

Update CHANGELOG.md

cd0afef

cnpryer force-pushed the publish branch from 8c3a8c8 to cd0afef Compare May 1, 2023 19:51

Improve positional dist arguments for publish command

d484931

cnpryer force-pushed the publish branch from 5425dac to d484931 Compare May 1, 2023 21:28

cnpryer marked this pull request as ready for review May 1, 2023 21:28

cnpryer commented May 1, 2023

View reviewed changes

rye/src/cli/publish.rs Outdated Show resolved Hide resolved

Update rye/src/cli/publish.rs

60594d0

Co-authored-by: Chris Pryer <14341145+cnpryer@users.noreply.github.com>

cnpryer marked this pull request as draft May 2, 2023 11:23

cnpryer force-pushed the publish branch 2 times, most recently from c7a1549 to 46379b0 Compare May 3, 2023 04:06

Start token process work

fad92b1

cnpryer force-pushed the publish branch from 46379b0 to fad92b1 Compare May 3, 2023 04:41

cnpryer added 10 commits May 3, 2023 00:44

Fix command description

e1c797d

Fix arg and input conflict

43b5738

Fix publish failure message

cbc8368

Add ring

634dec4

TODO(cnpryer): Finish experimenting

Merge branch 'main' into publish

03f67e4

Update utf-8 parse failure message

7d87b6f

Small chore

76be144

Use tables in credentials file

df8c92e

Small chore

563c20f

Merge imports

Remove .pypirc usage

f4ecb2e

TODO(cnpryer): Could probably still support .pypirc

cnpryer force-pushed the publish branch from 05afeb9 to 606539b Compare May 4, 2023 18:57

Merge branch 'main' into publish

7b30e62

cnpryer marked this pull request as ready for review May 9, 2023 18:30

cnpryer commented May 9, 2023

View reviewed changes

cnpryer commented May 10, 2023

View reviewed changes

cnpryer added 5 commits May 9, 2023 22:17

Use Url in publish command arg parsing

ba19eff

Don't always encode credential tokens

abd82b2

Merge branch 'main' into publish

a4553dd

Use dialoguer for passphrase prompt

b86d858

Merge branch 'main' into publish

b87b5d0

cnpryer force-pushed the publish branch from 1a6558d to b87b5d0 Compare May 11, 2023 16:44

cnpryer added 2 commits May 11, 2023 20:20

Merge branch 'main' into publish

6880bec

Merge branch 'main' into publish

a543903

mitsuhiko requested changes May 12, 2023

View reviewed changes

cnpryer added 3 commits May 12, 2023 17:00

Bump SELF_VERSION to 3

e828154

Use get_venv_bin_path

f6dc079

Fix publish url domain check

6c41cc3

cnpryer commented May 12, 2023

View reviewed changes

rye/src/cli/publish.rs Outdated Show resolved Hide resolved

cnpryer and others added 2 commits May 12, 2023 17:29

Remove unnecessary annotation

634b649

Merge branch 'main' into publish

a239e3e

mitsuhiko approved these changes May 12, 2023

View reviewed changes

mitsuhiko merged commit 10b0875 into astral-sh:main May 12, 2023
6 checks passed

cnpryer deleted the publish branch May 12, 2023 22:21

ischaojie pushed a commit to ischaojie/rye that referenced this pull request May 13, 2023

Add basic publish command (astral-sh#86)

9c49de4

CharlesChen0823 mentioned this pull request May 16, 2023

Add mkdocs gh links #172

Merged

cnpryer mentioned this pull request Feb 24, 2024

Fall back to Twine's keyring support in publish command #759

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic publish command #86

Add basic publish command #86

cnpryer commented May 1, 2023 •

edited

mitsuhiko commented May 2, 2023

cnpryer commented May 2, 2023 •

edited

mitsuhiko commented May 2, 2023

cnpryer commented May 2, 2023

cnpryer commented May 3, 2023

cnpryer commented May 9, 2023 •

edited

mitsuhiko commented May 9, 2023

cnpryer commented May 9, 2023 •

edited

cnpryer May 9, 2023

cnpryer May 10, 2023

cnpryer left a comment •

edited

cnpryer May 10, 2023

cnpryer May 10, 2023

cnpryer May 10, 2023

cnpryer May 10, 2023

cnpryer May 10, 2023 •

edited

cnpryer May 10, 2023

cnpryer May 10, 2023

cnpryer May 10, 2023

cnpryer May 10, 2023

cnpryer commented May 12, 2023

mitsuhiko May 12, 2023

cnpryer May 12, 2023

cnpryer May 12, 2023

mitsuhiko May 12, 2023

cnpryer May 12, 2023

cnpryer May 12, 2023

mitsuhiko commented May 12, 2023

		#[arg(long, default_value = "https://upload.pypi.org/legacy/")]
		repository_url: String,

Add basic publish command #86

Add basic publish command #86

Conversation

cnpryer commented May 1, 2023 • edited

How it works:

Summary of changes:

TODO:

Considerations

mitsuhiko commented May 2, 2023

cnpryer commented May 2, 2023 • edited

mitsuhiko commented May 2, 2023

cnpryer commented May 2, 2023

cnpryer commented May 3, 2023

cnpryer commented May 9, 2023 • edited

mitsuhiko commented May 9, 2023

cnpryer commented May 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnpryer left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnpryer May 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnpryer commented May 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitsuhiko commented May 12, 2023

cnpryer commented May 1, 2023 •

edited

cnpryer commented May 2, 2023 •

edited

cnpryer commented May 9, 2023 •

edited

cnpryer commented May 9, 2023 •

edited

cnpryer left a comment •

edited

cnpryer May 10, 2023 •

edited