-
Notifications
You must be signed in to change notification settings - Fork 300
enhance: provide contexts to credential helpers when listing credentials #979
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
enhance: provide contexts to credential helpers when listing credentials #979
Conversation
Re-requesting review because I added a test. |
Signed-off-by: Grant Linville <[email protected]>
Signed-off-by: Grant Linville <[email protected]>
Signed-off-by: Grant Linville <[email protected]>
Signed-off-by: Grant Linville <[email protected]>
Signed-off-by: Grant Linville <[email protected]>
9b2f066
to
300bc5f
Compare
@@ -53,7 +53,7 @@ init-docs: | |||
|
|||
# Ensure docs build without errors. Makes sure generated docs are in-sync with CLI. | |||
validate-docs: gen-docs | |||
docker run --rm --workdir=/docs -v $${PWD}/docs:/docs node:18-buster yarn build | |||
docker run --rm --workdir=/docs -v $${PWD}/docs:/docs node:18-buster npm run build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this for consistency with the other change from yarn to npm in the init-docs
target. Plus it wasn't working with Node v18 and yarn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 on tests
for obot-platform/obot#3180
This PR changes the way we request a list of credentials by also including the contexts as input. The contexts are ignored by all non-database credential helpers (like Wincred and macOS Keychain), so we still filter by context after we call
GetAll
, but this allows the database credential helpers to be more efficient and only return to us the credentials that we actually need.Once this PR is merged, I will have a PR for Obot to bump the GPTScript dependency. We might have to temporarily (or permanently, as I don't think it will do any harm) enable schema migration for the credentials table, which is currently disabled with an environment variable.
Related PR to change the credential helpers: obot-platform/tools#692