Skip to content
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

Fix/ghtoken #394

Closed
wants to merge 4 commits into from
Closed

Fix/ghtoken #394

wants to merge 4 commits into from

Conversation

leongross
Copy link
Contributor

@leongross leongross commented Nov 13, 2023

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

Sounds good to me 👍

@SergioGasquez
Copy link
Member

Sounds good to me 👍

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

@leongross
Copy link
Contributor Author

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

@SergioGasquez
Copy link
Member

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.

@bugadani
Copy link
Contributor

bugadani commented Dec 9, 2024

Just a gentle ping @leongross, do you intend to work further on this PR, or should we take over?

@leongross
Copy link
Contributor Author

Currently, I am swamped with other work, so feel free to take that PR over :)

@SergioGasquez
Copy link
Member

Just created #464 based on your work, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants