esp-rs / espup

Tool for installing and maintaining Espressif Rust ecosystem.
Apache License 2.0
224 stars 23 forks source link

Fix/ghtoken #394

Open leongross opened 11 months ago

leongross commented 11 months ago

When the environment contains a github token (that is read automatically) which is not valid anymore, the github API request will fail, leaving the CLI with an unclear error about the version.

This PR introduces a new error type for failure when logging into the API and will use a hardcoded fallback version of a known good toolchain. This version should be bumped regularly (Idea: maybe use dpendabot for that?)

SergioGasquez commented 11 months ago

Hi! Thanks for your contribution! Do you mind elaborating on your scenario a bit? Why would the environment contain a not valid GITHUB_TOKEN? There is a -k/--skip-version-parse argument that can be used to avoid querying GitHub API (this requires providing a -v/--toolchain-version)

This PR introduces a new error type for failure when logging into the API and will use a hardcoded fallback version of a known good toolchain. I don't really like having to update this every new release,

leongross commented 11 months ago

Hi! Sure: The scenario I ran into was that my ~/.zshrc exported a github token GITHUB_TOKEN, which was once valid but then expired. This lead to an invalid gh query for the version, in which the API response which contained "Bad credentials". This was not caught, but only the version parsing failed without any other warning.

It took me some time to figure that out, so added this check. I think catching this case is a good idea not to confuse users.

SergioGasquez commented 11 months ago

Thanks for the details! In this case, I think adding an error for it is beneficial, but I wouldn't proceed with the installation. I would just return the new GithubTokenInvalid error and end there. This would avoid requiring a fallback version.

leongross commented 11 months ago

Sounds good to me :+1:

SergioGasquez commented 11 months ago

Sounds good to me 👍

Do you mind implementing the required changes, or you want me to supersede this PR?

leongross commented 11 months ago

I'll take care of it in the next couple of days :)

SergioGasquez commented 11 months ago

I'll take care of it in the next couple of days :)

Awesome, thanks! There is no hurry, so do it at your own pace.