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.

Sign up for GitHub

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

Jump to bottom

feat: saved api_key to keychain for user #104

Merged
merged 7 commits into from
Oct 6, 2024
Merged

feat: saved api_key to keychain for user #104

merged 7 commits into from
Oct 6, 2024

Conversation

lifeizhou-ap
Copy link
Collaborator

@lifeizhou-ap lifeizhou-ap commented Oct 1, 2024 •
edited
Loading

Why
Enable users to store api key in keychain for convenience

What

Rules
When the api key environment variable does not exist,

  • it looks up the api key from keychain. if it does not exist, then
  • the users are prompted to enter the api key and
  • the users are prompted whether they want to save the api key in the keychain

Changes

  • Implemented rules as above
  • Extract create_exchange into a util file
  • Moved SessionNotifier to a separate file

Screenshot 2024-10-01 at 4 08 35 PM
Screenshot 2024-10-01 at 4 09 20 PM
Screenshot 2024-10-01 at 4 09 44 PM

Screenshot 2024-10-04 at 3 17 32 PM

@lamchau
Copy link
Collaborator

lamchau commented Oct 1, 2024 •
edited
Loading

very cool! how would a user update the keychain with the same prompts (typo, rotation, etc)?

just had a thought! what do you think of a goose config (and maybe even a goose check)? that way we can use it as a catch all to do things

  • check latest version
  • check profiles
  • check api keys of each providers (both environment variables and keychain)?

@michaelneale
Copy link
Collaborator

cc @alecthomas

@lifeizhou-ap
Merge branch 'main' into lifei/keychain
ed5b52d 
* main:
  fix: exit the goose and show the error message when provider environment variable is not set (#103)
  fix: Update OpenAI pricing per https://openai.com/api/pricing/ (#110)
  fix: update developer tool prompts to use plan task status to match allowable statuses update_plan tool call (#107)
  fix: removed the panel in the output so that the user won't have unnecessary pane borders in the copied content (#109)
  docs: update links to exchange to the new location (#108)
  chore: setup workspace for exchange (#105)
@lifeizhou-ap
fixed merge issue and added test cases
b32b1ea
lifeizhou-ap
lifeizhou-ap commented Oct 4, 2024
View reviewed changes
src/goose/cli/session.py
@@ -52,24 +47,6 @@ def load_profile(name: Optional[str]) -> Profile:
return profile


class SessionNotifier(Notifier):
Copy link
Collaborator Author

@lifeizhou-ap lifeizhou-ap Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

moved to a seperate file

lifeizhou-ap
lifeizhou-ap commented Oct 4, 2024
View reviewed changes
src/goose/cli/session.py
@@ -102,21 +81,6 @@ def __init__(

self.prompt_session = GoosePromptSession()

def _create_exchange(self) -> Exchange:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

exacted to a util function

@lifeizhou-ap
Copy link
Collaborator Author

lifeizhou-ap commented Oct 4, 2024 •
edited
Loading

just had a thought! what do you think of a goose config (and maybe even a goose check)? that way we can use it as a catch all to do things

  • check latest version
  • check profiles
  • check api keys of each providers (both environment variables and keychain)?

Yes, that could be useful.

@lifeizhou-ap
Copy link
Collaborator Author

lifeizhou-ap commented Oct 4, 2024 •
edited
Loading

how would a user update the keychain with the same prompts (typo, rotation, etc)?

I think we can enhance this later. When we load the api key from keychain, the console prints the message about the api_key source. If it is not correct, the user still can update by themselves.

@lifeizhou-ap lifeizhou-ap marked this pull request as ready for review October 4, 2024 05:18
@alecthomas
Copy link

This is awesome!

@lifeizhou-ap lifeizhou-ap merged commit f3aab12 into main Oct 6, 2024
4 checks passed
@lifeizhou-ap lifeizhou-ap deleted the lifei/keychain branch October 7, 2024 03:13
lily-de pushed a commit that referenced this pull request Oct 7, 2024
@lifeizhou-ap @lily-de
feat: saved api_key to keychain for user (#104)
8344b6a
lukealvoeiro added a commit that referenced this pull request Oct 9, 2024
@lukealvoeiro
Merge branch 'main' into use-lsp
61d36c7 
* main: (41 commits)
  chore: Add goose providers list command (#116)
  docs: working ollama for desktop (#125)
  docs: format and clean up warnings/errors (#120)
  docs: update deploy workflow (#124)
  feat: Implement a goose run command (#121)
  feat: saved api_key to keychain for user (#104)
  docs: add callout plugin (#119)
  chore: add a page to docs for Goose application examples (#117)
  fix: exit the goose and show the error message when provider environment variable is not set (#103)
  fix: Update OpenAI pricing per https://openai.com/api/pricing/ (#110)
  fix: update developer tool prompts to use plan task status to match allowable statuses update_plan tool call (#107)
  fix: removed the panel in the output so that the user won't have unnecessary pane borders in the copied content (#109)
  docs: update links to exchange to the new location (#108)
  chore: setup workspace for exchange (#105)
  fix: resolve uvx when using a git client or IDE (#98)
  ci: add include-markdown for mkdocs (#100)
  chore: fix broken badge on readme (#102)
  feat: add global optional user goosehints file (#73)
  docs: update docs (#99)
  chore(release): release 0.9.3 (#97)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lily-de lily-de lily-de approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lifeizhou-ap @lamchau @michaelneale @alecthomas @lily-de