-
Notifications
You must be signed in to change notification settings - Fork 3
add frontend panel and add docker instructions #8
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
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.
Thanks @Jas0nch for working on this.
I left some comments below.
.gitignore
Outdated
@@ -123,3 +123,6 @@ dmypy.json | |||
|
|||
# Yarn cache | |||
.yarn/ | |||
|
|||
# Test results | |||
junit.xml |
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.
junit.xml | |
junit.xml | |
style/base.css
Outdated
|
||
.jp-SecretsPanel-add-form-buttons button { | ||
flex: 1; | ||
} |
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.
} | |
} | |
@@ -34,7 +43,16 @@ const manager: JupyterFrontEndPlugin<ISecretsManager> = { | |||
connector: ISecretsConnector | |||
): ISecretsManager => { | |||
console.log('JupyterLab extension jupyter-secrets-manager is activated!'); | |||
return new SecretsManager({ connector }); | |||
const secretsManager = new SecretsManager({ connector }); | |||
const panel = new Panel(); |
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 should probably build the side panel in another plugin, to be able to disable it.
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.
Is it too much to have the UI in another plugin? Do you have a use case in mind where we want to enable the secrets manage functionality but disable the UI?
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.
It already works to automatically populate inputs without UI, see #8 (comment).
src/index.ts
Outdated
const secretsManager = new SecretsManager({ connector }); | ||
const panel = new Panel(); | ||
panel.id = 'jupyter-secrets-manager:panel'; | ||
panel.title.icon = logoIcon; |
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.
There is already a lock icon in Jupyterlab, we should reuse it.
import { lockIcon } from '@jupyterlab/ui-components';
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.
good catch, I'll change to reuse that.
I wonder how we could handle namespaces in the panel. Maybe for a first implementation, the secrets manager could provide a list of existing namespaces, which would populate a drop down in the widget. |
Thanks @brichet to review my PR. I agree with you on this. Actually I'm thinking should we make it further to notebook level? It would be great to have the capability to grant/revoke a secrets to a notebook to get more fine-grained access control. But namespace level can be the first implementation. I'm also very curious on the vision of this plugin, in the long run, how would it work? It will populate the secrets to kernel, right? It will also accept other plugin's code to add secrets, right? Anything else? Is there a documentation? I was reading jupyterlab/jupyter-ai#1237 and found that there's already something developing. So I'm thinking if we should gather some input from jupyter-ai team to see how the secrets manager could be evolved. |
Indeed, we mentioned it quickly during a jupyter-ai meeting but did not mention it in the
Currently it works in the frontend only, but we should indeed think of a way to send the secrets to the kernel.
The way it works currently (frontend only):
For example, this is used in jupyterlite-ai, from this PR. Plugins should probably not add secrets, but rather allow associating a namespace/ID to a secret by the user. |
Let me check the usage and think about how can I improve this PR |
manager: ISecretsManager; | ||
} | ||
|
||
const EyeIcon = () => ( |
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 didn't find a good import for these eye icons, please let me know if any better eye icons
Hi @brichet , I read the jupyterlite-ai usage of the secrets manager and now I understand how it works. I have update a new version of the PR along with the description and demo, please take a look when you have time. I still have some questions and want to discuss in the early stage as it's more flexible. Some open questions regarding this PR:
Some proposals regarding this plugin:
|
I agree with you. Let’s hold this PR for now |
This Pull Request contains the following changes:
jlpm test
There're also a lot of Todos/Improvements by introducing this Pull Request:
jupyter-secrets-manager-side-panel.mov