-
Notifications
You must be signed in to change notification settings - Fork 6
Add a default connector plugin #1
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
Conversation
e399ffa to
b30dae5
Compare
|
Nice, this looks good. FYI #2 should include a couple of CI fixes, if that can help. |
b30dae5 to
b948eb4
Compare
src/connectors/in-memory.ts
Outdated
| ids.push(key); | ||
| } | ||
| }); | ||
| return { ids, values: [] }; |
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.
Should the values contain the actual secrets?
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.
We could, but currently it would be useless because of the interface of the SecretManager
jupyter-secrets-manager/src/token.ts
Line 21 in f909d00
| list(namespace: string): Promise<string[]>; |
jupyter-secrets-manager/src/manager.ts
Lines 25 to 27 in f909d00
| async list(namespace: string): Promise<string[]> { | |
| return (await this._connector.list(namespace)).ids; | |
| } |
I would be useful if there was a UI displaying all the secrets.
Maybe we can change it now (probably in another PR to also update the SecretManager), to avoid future breaking changes.
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.
ok, probably fine either way.
For now the most important is being able to set and get individual secrets.
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.
OK, I updated it and opened #3
|
Thanks for the review @jtpio |
* Add a default connector * Connector list() method returns also the values
This PR adds a default connector plugin, storing the password in memory.
The passwords live only during the current session.