Implement auth and transfer commands #1298

f-f · 2024-11-01T22:51:37Z

We fix #1123 by adding the auth and registry transfer commands, to allow for authenticated interactions with the Registry. The unpublish command is still unimplemented, but it should be easy now that the rest of the scaffolding is in place.

This PR includes a whole set of tests to verify that things do what they should, and docs to inform users how the authenticated flow looks like.

@fsoikin I intend to merge this before #1296 since I've been rebasing it for months already and I'd love to not have to do it once more 😄

(this fixes #1294 as well)

fsoikin · 2024-11-02T00:26:55Z

Absolutely no problem, I assumed it would be this way. I'm happy to keep rebasing until it's time.

fsoikin · 2024-11-02T00:29:48Z

I'm afraid it'll take me a few days to review though. This is quite large.

f-f · 2024-11-03T17:47:57Z

@fsoikin no worries! That'll give me some time to have a good look at #1296 🙂

f-f · 2024-11-03T17:58:37Z

Of course happy to have @thomashoneyman's review as well here if it doesn't take away time from other things 🙂

thomashoneyman

This looks good to me! I don't know if I have enough fresh context on Spago to give an approval, but everything seems reasonable. Just a couple comments.

thomashoneyman · 2024-11-07T21:21:42Z

bin/src/Main.purs

-    , commandParser "fetch" (Fetch <$> fetchArgsParser) "Downloads all of the project's dependencies"
-    , commandParser "install" (Install <$> installArgsParser) "Compile the project's dependencies"
-    , commandParser "uninstall" (Uninstall <$> uninstallArgsParser) "Remove dependencies from a package"
+    [ commandParser "auth" (Auth <$> authArgsParser) "Authenticate as the owner of a package, to allow transfer and unpiblish operations"


Suggested change

[ commandParser "auth" (Auth <$> authArgsParser) "Authenticate as the owner of a package, to allow transfer and unpiblish operations"

[ commandParser "auth" (Auth <$> authArgsParser) "Authenticate as the owner of a package, to allow transfer and unpublish operations"

thomashoneyman · 2024-11-07T21:27:00Z

src/Spago/Command/Auth.purs

+      Just _rest -> keyPath
+      Nothing -> keyPath <> ".pub"
+
+  newOwner <- FS.exists path >>= case _ of


When I first read this file I was wondering why this was necessarily a "new owner", until I saw that this function is specifically intended as a command for adding a new key. Could that be added as a module comment so someone coming into this file without a lot of context knows its purpose?

thomashoneyman · 2024-11-07T21:27:27Z

src/Spago/Command/Fetch.purs

@@ -254,7 +254,7 @@ fetchPackagesToLocalCache packages = do
            case tarExists, tarIsGood, offline of
              true, true, _ -> pure unit -- Tar exists and is good, and we already unpacked it. Happy days!
              _, _, Offline -> die $ "Package " <> packageVersion <> " is not in the local cache, and Spago is running in offline mode - can't make progress."
-              _, _, Online -> do
+              _, _, _ -> do


No more love for exhaustive cases? 😆

fsoikin · 2024-11-08T04:44:20Z

README.md

+Authentication and operations that use it are automated by Spago, through the `spago auth` command: if you'd like to
+be able to perform authenticated operations you need a SSH keypair, and run `spago auth` passing those keys in.
+This will populate the `package.publish.owners` field in the `spago.yaml` - commit that and publish a new version,
+and from that moment you'll be able to perform authenticated operations on this package.


Does this mean that I can "take over" somebody else's package by doing this before they get to it?

Oh, this is dependent on me having the right to commit the public key to the repo?

fsoikin · 2024-11-08T04:46:53Z

README.md

+The transfer procedure is automated by Spago commands, and goes as follows:
+* Add your (or the new owner's) SSH public key to the `spago.yaml` through `spago auth` if they are not there already (see previous section)
+* Transfer the repository to the new owner using the hosting platform's transfer mechanism (e.g. GitHub's transfer feature)
+* Depending whose key is present in the `owners` field, either you or the new owner will update the `publish.location` field in the `spago.yaml`, and call `spago registry transfer` to initiate the transfer. If all goes well you'll now be able to publish a new version from the new location.


Suggested change

* Depending whose key is present in the `owners` field, either you or the new owner will update the `publish.location` field in the `spago.yaml`, and call `spago registry transfer` to initiate the transfer. If all goes well you'll now be able to publish a new version from the new location.

* Depending on whose key is present in the `owners` field, either you or the new owner will update the `publish.location` field in the `spago.yaml`, and call `spago registry transfer` to initiate the transfer. If all goes well you'll now be able to publish a new version from the new location.

fsoikin

Looks fluffy!

fsoikin · 2024-11-08T04:52:09Z

src/Spago/Command/Auth.purs

+  { doc, package, configPath } <- case workspace.selected, workspace.rootPackage of
+    Just { doc, package, path: packagePath }, _ -> pure { doc, package, configPath: Path.concat [ packagePath, "spago.yaml" ] }
+    Nothing, Just rootPackage -> pure { doc: workspace.doc, package: rootPackage, configPath: "spago.yaml" }
+    Nothing, Nothing -> die "No package was selected. Please select a package with the -p flag"


I feel like this should be a reusable thing.

fsoikin · 2024-11-08T04:53:17Z

src/Spago/Command/Auth.purs

+      case Array.elem newOwner currentOwners of
+        true -> logWarn "The selected key is already present in the config file."
+        false -> do
+          logInfo $ "Adding the selected key  to the list of the owners: " <> path


Suggested change

logInfo $ "Adding the selected key to the list of the owners: " <> path

logInfo $ "Adding selected key to the list of the owners: " <> path

fsoikin · 2024-11-08T04:53:24Z

src/Spago/Command/Auth.purs

+    Just { owners: maybeOwners } -> do
+      let currentOwners = fromMaybe [] maybeOwners
+      case Array.elem newOwner currentOwners of
+        true -> logWarn "The selected key is already present in the config file."


Suggested change

true -> logWarn "The selected key is already present in the config file."

true -> logWarn "Selected key is already present in the config file."

fsoikin · 2024-11-08T04:54:54Z

src/Spago/Command/Publish.purs

+          , toDoc "Location:"
+          , indent (toDoc $ "- " <> prettyPrintLocation location)
+          , toDoc "Remotes:"
+          , lines $ map (\r -> indent $ toDoc $ "- " <> r.name <> ": " <> r.url) locationResult.remotes


Suggested change

, lines $ map (\r -> indent $ toDoc $ "- " <> r.name <> ": " <> r.url) locationResult.remotes

, lines $ locationResult.remotes <#> \r -> indent $ toDoc $ "- " <> r.name <> ": " <> r.url

fsoikin · 2024-11-08T04:57:10Z

src/Spago/Command/Registry.js

@@ -0,0 +1,7 @@
+import readlineSync from 'readline-sync';
+
+export function question(query) {


Since this specifically uses hideEchoBack: true, perhaps give it a less generic name? Like questionPassword or something? Or, alternatively, take the hideEchoBack parameter from up the stack?

fsoikin · 2024-11-08T05:02:41Z

test-fixtures/1146-cli-help/build.txt

-                          [--pedantic-packages] [--pure] [--purs-args ARGS]
-                          [-p|--package PACKAGE] ([--verbose-stats] |
-                          [--censor-stats]) [--strict]
+Usage: index.dev.js build [--migrate] [--monochrome|--no-color] [--offline] [-q|--quiet] [-v|--verbose] [--backend-args ARGS] [--ensure-ranges] [--json-errors] [--output DIR] [--pedantic-packages] [--pure] [--purs-args ARGS] [-p|--package PACKAGE] ([--verbose-stats] | [--censor-stats]) [--strict]


What happened to the line wrapping?

fsoikin · 2024-11-08T05:03:19Z

test-fixtures/1146-cli-help/registry-search.txt

@@ -14,6 +12,5 @@ Available options:
  -q,--quiet               Suppress all spago logging
  -v,--verbose             Enable additional debug logging, e.g. printing `purs`
                           commands
-  --json                   Format the output as JSON
-  PACKAGE                  Package name
+  --json                   Format the output as JSON PACKAGE                  Package name


Implement auth and transfer commands

4346ab8

f-f requested a review from fsoikin November 1, 2024 22:51

Update package lock

1d2cfc0

Fix test fixture

4beb246

thomashoneyman reviewed Nov 7, 2024

View reviewed changes

fsoikin reviewed Nov 8, 2024

View reviewed changes

fsoikin approved these changes Nov 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement auth and transfer commands #1298

Implement auth and transfer commands #1298

f-f commented Nov 1, 2024 •

edited

Loading

fsoikin commented Nov 2, 2024

fsoikin commented Nov 2, 2024

f-f commented Nov 3, 2024

f-f commented Nov 3, 2024

thomashoneyman left a comment •

edited

Loading

thomashoneyman Nov 7, 2024

thomashoneyman Nov 7, 2024

thomashoneyman Nov 7, 2024

fsoikin Nov 8, 2024

fsoikin Nov 8, 2024

fsoikin Nov 8, 2024

fsoikin left a comment

fsoikin Nov 8, 2024

fsoikin Nov 8, 2024

fsoikin Nov 8, 2024

fsoikin Nov 8, 2024

fsoikin Nov 8, 2024

fsoikin Nov 8, 2024

fsoikin Nov 8, 2024

	[ commandParser "auth" (Auth <$> authArgsParser) "Authenticate as the owner of a package, to allow transfer and unpiblish operations"
	[ commandParser "auth" (Auth <$> authArgsParser) "Authenticate as the owner of a package, to allow transfer and unpublish operations"

	* Depending whose key is present in the `owners` field, either you or the new owner will update the `publish.location` field in the `spago.yaml`, and call `spago registry transfer` to initiate the transfer. If all goes well you'll now be able to publish a new version from the new location.
	* Depending on whose key is present in the `owners` field, either you or the new owner will update the `publish.location` field in the `spago.yaml`, and call `spago registry transfer` to initiate the transfer. If all goes well you'll now be able to publish a new version from the new location.

	logInfo $ "Adding the selected key to the list of the owners: " <> path
	logInfo $ "Adding selected key to the list of the owners: " <> path

	true -> logWarn "The selected key is already present in the config file."
	true -> logWarn "Selected key is already present in the config file."

	, lines $ map (\r -> indent $ toDoc $ "- " <> r.name <> ": " <> r.url) locationResult.remotes
	, lines $ locationResult.remotes <#> \r -> indent $ toDoc $ "- " <> r.name <> ": " <> r.url

		@@ -0,0 +1,7 @@
		import readlineSync from 'readline-sync';

		export function question(query) {

Implement auth and transfer commands #1298

Are you sure you want to change the base?

Implement auth and transfer commands #1298

Conversation

f-f commented Nov 1, 2024 • edited Loading

fsoikin commented Nov 2, 2024

fsoikin commented Nov 2, 2024

f-f commented Nov 3, 2024

f-f commented Nov 3, 2024

thomashoneyman left a comment • edited Loading

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

Choose a reason for hiding this comment

fsoikin left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-f commented Nov 1, 2024 •

edited

Loading

thomashoneyman left a comment •

edited

Loading