On expired `NEIL_GITHUB_TOKEN`, `neil dep upgrade` does nothing, then terminates with exit code 0

teodorlu commented 3 months ago

Observation

When I run neil dep upgrade either from the command line with an expired NEIL_GITHUB_TOKEN, Neil prints error messages, but returns with exit code 0.

$ neil-dev dep upgrade
{"message":"Bad credentials","documentation_url":"https://docs.github.com/rest","status":"401"}
{"message":"Bad credentials","documentation_url":"https://docs.github.com/rest","status":"401"}
$ echo $?
0

Expected behavior

I would expect neil dep upgrade to terminate with a nonzero exit code when nothing happened due to an expired Github token.

Perhaps also give a more helpful description than printing

{"message":"Bad credentials","documentation_url":"https://docs.github.com/rest","status":"401"}

twice, informing the user that their NEIL_GITHUB_TOKEN didn't appear to work.

Extra details

This is only likely to hit Neil developers, users are not encouraged to setup their own github token for neil.
This also appears to affect unit tests. I hit this first by writing a unit tests an not seeing anything update. (Details: https://github.com/babashka/neil/pull/236/files#diff-971ad24024ab537d1b6a3c2d0c37956be2ea8cdf52057b474f150b24d7c3b968R312-R326)

borkdude commented 3 months ago

sounds good

teodorlu commented 3 months ago

Perhaps a better alternative could be to print a warning, then retry without authentication.

Preference, @borkdude?

borkdude commented 3 months ago

sounds good as well!

teodorlu commented 3 months ago

Perhaps a better alternative could be to print a warning, then retry without authentication.

It appears this won't work.

From local testing, invalid Github token works from Github's API. The HTTP request only starts returning non-success HTTP responses after the free usage has been consumed. In other words, retrying won't help, we'll just trigger a different error; not helpful for the user.

I thought of something else that further complicates the issue. With the following deps.edn file, there are both Git deps and Clojars deps:

{:deps {babashka/pods {#_#_:local/root "/Users/borkdude/Dropbox/dev/clojure/babashka/babashka.pods"
                       :git/url "https://github.com/babashka/babashka.pods"
                       :git/sha "47e55fe5e728578ff4dbf7d2a2caf00efea87b1e"}
        io.github.nextjournal/clerk {:mvn/version "0.10.560"}}
 :aliases {}}

Current neil dep upgrade behavior is to "do the things we can": upgrade Clerk to latest, and leave babashka/pods alone because the HTTP request to Github failed.

I don't think changing this behavior to "Neil refuses to do anything because one request to Github failed" is helpful for the user.

Proposed new scope:

Add an error message for invalid credentials
Leave current exit code behavior as-is.

babashka / neil