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

Created --reset-key flag to update OpenAI key #395

Merged
merged 6 commits into from
Dec 19, 2023

Conversation

Ismail-Ben
Copy link
Contributor

@Ismail-Ben Ismail-Ben commented Dec 12, 2023

Resolves: #30

Added --reset-key flag to update OPENAI_API_KEY in .sgptrc file. Also verified with a dummy chat completion that the newly inputted key works.

Sample output with 2 non-valid keys followed by a real key:
image

@Ismail-Ben
Copy link
Contributor Author

@TheR1D Any suggested changes to get this in?

Copy link
Owner

@TheR1D TheR1D left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this option doesn't align with prioritized environment variables in config now, since we can't reset user exported OPENAI_API_KEY. But we could add better error handling into client._request simply checking respone status code for 403/401 printing the error text and raising Exit with error raise typer.Exit(code=1).

@Ismail-Ben
Copy link
Contributor Author

Ismail-Ben commented Dec 13, 2023

I think this option doesn't align with prioritized environment variables in config now, since we can't reset user exported OPENAI_API_KEY. But we could add better error handling into client._request simply checking respone status code for 403/401 printing the error text and raising Exit with error raise typer.Exit(code=1).

I've moved the error handling to client._request as suggested.

As for the reset functionality I added a note to the "help" section of the command as a reminder that the environment variable will still be prioritized. I still believe having a --reset-key option is worthwhile as I would've liked to use it myself when first trying the tool. Instead of having to navigate to the .sgptrc file and update it manually it would've been nice to have this command.

The config.get() function properly prioritizes the use of the env key if it exists so updating the .sgptrc file wouldn't change this behavior. In the case that a user runs this command while already having the env variable, I highlight in yellow that their env key will still be used.

User updates key with env variable defined:
image

No env variable defined, and fake key:
image

Help section:
image

Let me know what you think @TheR1D . Thanks!

Copy link
Owner

@TheR1D TheR1D left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is very common in CLI applications to use runtime configuration file or ENV variables to set API keys (without having dedicated CLI options to reset some of config values). Please remove reset key option, if we show user proper message, it is clear how to reset it.

sgpt/client.py Outdated Show resolved Hide resolved
sgpt/client.py Outdated Show resolved Hide resolved
sgpt/client.py Outdated Show resolved Hide resolved
@Ismail-Ben
Copy link
Contributor Author

Adjusted to reflect @TheR1D 's comments. When using an invalid key the output now looks like:
image

TheR1D
TheR1D previously approved these changes Dec 14, 2023
Copy link
Owner

@TheR1D TheR1D left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @Ismail-Ben

@TheR1D
Copy link
Owner

TheR1D commented Dec 14, 2023

Please check lint issues.

@TheR1D TheR1D merged commit 0a6fbe5 into TheR1D:main Dec 19, 2023
3 checks passed
@TheR1D
Copy link
Owner

TheR1D commented Dec 19, 2023

Thank you, merged.

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.

Implement API key reset option
2 participants