Skip to content

Conversation

@lily-de
Copy link
Contributor

@lily-de lily-de commented Mar 6, 2025

One change that touched many files so it looks bigger than it is:

All pre-existing uses of set are now set_param and uses of get are get_param -- same function, just different name now

Before

  • get(key) -- the config method to fetch a non-secret
  • get_secret(key) -- the config method to fetch a secret
  • set(key, value) -- the config method to set a non-secret
  • set_secret(key, value) -- the config method to set a secret

After

  • get_param(key) -- the config method to fetch a non-secret
  • get_secret(key) -- the config method to fetch a secret
  • set_param(key, value) -- the config method to set a non-secret
  • set_secret(key, value) -- the config method to set a secret
  • get(key, is_secret) -- the config method to fetch any key -- uses get_param and get_secret based on value of is_secret
  • set(key, value, is_secret) -- the config method to get any key -- uses set_param and set_secret based on value of is_secret

Why?

  • having a get function to always check all places for a secret makes it less complicated for the config endpoints that the frontend uses. we still never return the actual value of the secrets, but this way we have one method to call and it will handle the lookup or setting
  • the frontend uses is_secret for setting and getting parameters a lot, so this creates parity as well

Rest of changes

  1. Created an endpoint to return provider metadata and configuration state for each provider
  2. Had to do some refactoring of the existing providers settings to handle the new datatype returned by the endpoint
  3. Also had fix the page endlessly re-rendering and was suggested by goose to add some memo-ization

State of new settings at end of this PR:

  • getting the provider information and state ✅
  • we fetch the state and details about all providers from the backend, and now the frontend is using the same information as the cli
  • note: we have to calculate 'is_configured' a bit differently than the cli because the cli checks the state of the provider's keys as the user goes through the configure flow. in the future we could refactor this so that there's shared functions between the two

Next:

  • setting the provider information and state

@lily-de lily-de force-pushed the ldelalande/config-and-providers branch from a089878 to 2ad2b62 Compare March 7, 2025 19:48
@lily-de lily-de marked this pull request as ready for review March 7, 2025 20:12
@lily-de lily-de force-pushed the ldelalande/config-and-providers branch from 30e174d to 3f3f430 Compare March 9, 2025 17:49
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some more routes and schemas so openapi can create client-side functions for the UI to use to avoid needing to generate the types and client functions to call the backend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the real changes are here -- added an endpoint called providers which fetches provider metadata (name, description, keys etc) and if each provider is configured or not and returns that to the frontend

also updated other endpoints to verify that there's a secret key in the headers of the request for security -- this is something we do currently when fetching information on providers and extensions but we forgot to include it in the first iteration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

helper functions for the providers endpoint -- looks up information on if keys are set or not for the providers and determines if all required keys are set or not. will return key value in the response if not secret

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just renamed get to get_param and set to set_param

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the get(key, is_secret) and set(key, value, is_secret) methods and renamed the old get(key) and set(key, value) functions to get_param and set_param

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file is auto-generated -- has the some updated information for the frontend on the different routes and schemas

Copy link
Collaborator

@michaelneale michaelneale left a comment

Choose a reason for hiding this comment

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

yes looks like a big refactor!

@lily-de lily-de merged commit 5df2875 into main Mar 10, 2025
6 checks passed
@lily-de lily-de deleted the ldelalande/config-and-providers branch March 10, 2025 16:51
michaelneale added a commit that referenced this pull request Mar 11, 2025
* main:
  feat: enable smart approve for user by default (#1599)
  ui: fix modal state (#1598)
  ui: setting configuration (#1597)
  fix: merge error logging in goose bench  (#1545)
  feat: add additional goosebench evals (#1571)
  chore: update types and imports (#1594)
  Retain session through view changes (#1580)
  docs: Add steps for desktop tutorial (#1590)
  remove env vars from bottom menu model setting (#1584)
  Fix Goosehints modal UI (#1581)
  docs: typo fix (#1593)
  feat: update config endpoints for use with providers (#1563)
  fix: update anthropic provider headers (#1592)
  feat: Build Goose in a Docker Container (#1551)
  docs: voyp blog post (#1588)
sheagcraig added a commit to sheagcraig/goose that referenced this pull request Mar 11, 2025
* upstream/main: (48 commits)
  feat: enable smart approve for user by default (block#1599)
  ui: fix modal state (block#1598)
  ui: setting configuration (block#1597)
  fix: merge error logging in goose bench  (block#1545)
  feat: add additional goosebench evals (block#1571)
  chore: update types and imports (block#1594)
  Retain session through view changes (block#1580)
  docs: Add steps for desktop tutorial (block#1590)
  remove env vars from bottom menu model setting (block#1584)
  Fix Goosehints modal UI (block#1581)
  docs: typo fix (block#1593)
  feat: update config endpoints for use with providers (block#1563)
  fix: update anthropic provider headers (block#1592)
  feat: Build Goose in a Docker Container (block#1551)
  docs: voyp blog post (block#1588)
  fix: included files was panicing because dir didnt exist (block#1583)
  feat: work with docs/xls and simple html (block#1526)
  feat: parallel processing in approve mode (block#1575)
  Feat: support auto-including dirs in binary/bench-work-dir (block#1576)
  refactor models component (block#1535)
  ...
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