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.

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

plugin: Add plugin catalog plugin #35

Merged
merged 18 commits into from
Jul 25, 2024
Merged

plugin: Add plugin catalog plugin #35

merged 18 commits into from
Jul 25, 2024

Conversation

yolossn
Copy link
Contributor

@yolossn yolossn commented Apr 12, 2024

Plugin catalog is a plugin that helps
install and manage headlamp plugins
from artifacthub

For headlamp-k8s/headlamp#1654

Video.Project.1.mp4

How to use

From headlamp repo current main branch:

cd app
npm i
npm start

From plugins repo:

cd plugin-catalog
npm i
npm start

Now the Plugin Catalog link appears in the sidebar.

Copy link
Contributor

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

Thanks for the video. I took a quick look at the code but may have more comments later when I look again in more detail.
I also think that for the UX, instead of a dialog showing that the plugin is installing, we should have a progress bar with a cancel button which replace the install button in the plugin view.

plugin-store/.gitignore Outdated Show resolved Hide resolved
plugin-store/src/components/plugins/Detail.tsx Outdated Show resolved Hide resolved
Comment on lines 110 to 115
const headlampPlugin = runCommand('electron-node', [
'plugin-management.js',
'update',
pluginName,
'-j',
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be rather a new Headlamp. API call that does this transparently for the plugin.


registerSidebarEntry({
name: 'store',
url: '/plugin_store',
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the route as plugin-catalog.

@illume
Copy link
Contributor

illume commented Jun 18, 2024

Hrmm. There's a problem with the build command, which is causing the failure in CI. Which I think is unrelated to this PR.

Copy link
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

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

Nice one.

I did a first pass review, and left a few notes for your consideration.

A few more component stories would be appreciated for ones without.

I didn't run it myself and test locally yet.

plugin-catalog/src/components/plugins/List.tsx Outdated Show resolved Hide resolved
plugin-catalog/src/components/plugins/List.tsx Outdated Show resolved Hide resolved
plugin-catalog/README.md Outdated Show resolved Hide resolved
plugin-catalog/package.json Outdated Show resolved Hide resolved
plugin-catalog/dist/main.js Outdated Show resolved Hide resolved
plugin-catalog/src/components/plugins/Loading.stories.tsx Outdated Show resolved Hide resolved
@illume
Copy link
Contributor

illume commented Jun 18, 2024

I guess it makes sense to add artifact hub badges as well? (as app-catalog did recently)

@yolossn yolossn requested a review from illume July 1, 2024 10:13
@illume illume changed the title plugin: Add plugin store plugin plugin: Add plugin catalog plugin Jul 3, 2024
Copy link
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

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

Nice one.

Could you please add a "How to test" section in the PR description?

plugin-catalog/src/components/plugins/List.tsx Outdated Show resolved Hide resolved
plugin-catalog/src/components/plugins/List.tsx Outdated Show resolved Hide resolved
plugin-catalog/src/components/plugins/List.tsx Outdated Show resolved Hide resolved
@illume
Copy link
Contributor

illume commented Jul 3, 2024

I was thinking the "Store" part doesn't fit. Because it's not a store really. What do you think? /cc @joaquimrocha @blixtra

Just "Plugins" or "Plugin Catalog" ?

@illume
Copy link
Contributor

illume commented Jul 3, 2024

I'm getting this error:
image

Maybe it's this error:
http://localhost:4466/externalproxy, no allowed proxy url match, request denied

@yolossn
Copy link
Contributor Author

yolossn commented Jul 3, 2024

Maybe it's this error: http://localhost:4466/externalproxy, no allowed proxy url match, request denied

Add this to your env before running backend
export HEADLAMP_CONFIG_PROXY_URLS=https://artifacthub.io/*

@illume
Copy link
Contributor

illume commented Jul 3, 2024

We have a Settings -> Plugins (that looks like the screenshot here). I'm wondering if this should be inside there or not?

image

@illume
Copy link
Contributor

illume commented Jul 3, 2024

Maybe it's this error: http://localhost:4466/externalproxy, no allowed proxy url match, request denied

Add this to your env before running backend export HEADLAMP_CONFIG_PROXY_URLS=https://artifacthub.io/*

I guess this needs to be a default when the app is run?

Or at least cd app && npm start should be modified so it's a default.

@yolossn
Copy link
Contributor Author

yolossn commented Jul 3, 2024

I guess this needs to be a default when the app is run?

Or at least cd app && npm start should be modified so it's a default.

I usually run the backend separately when developing and add the env there, I am not sure if this should be added in the app/start.js. It gets added in the app/electron/main.ts automatically from the app-build-manifest.json

@illume
Copy link
Contributor

illume commented Jul 3, 2024

I guess this needs to be a default when the app is run?

Or at least cd app && npm start should be modified so it's a default.

I usually run the backend separately when developing and add the env there, I am not sure if this should be added in the app/start.js. It gets added in the app/electron/main.ts automatically from the app-build-manifest.json

Hrmm. I'm still getting an error with the proxy added.

ps. I made a PR for adding artifact hub proxy URL by default in the dev mode for 'make run-backend' headlamp-k8s/headlamp#2130

Copy link
Contributor

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

Left a few comments about the UX. The functionality worked fine for me when I tried it.

plugin-catalog/src/components/plugins/Detail.tsx Outdated Show resolved Hide resolved
plugin-catalog/src/components/plugins/Detail.tsx Outdated Show resolved Hide resolved
plugin-catalog/src/components/plugins/List.tsx Outdated Show resolved Hide resolved
plugin-catalog/src/components/plugins/List.tsx Outdated Show resolved Hide resolved
plugin-catalog/src/components/plugins/Detail.tsx Outdated Show resolved Hide resolved
plugin-catalog/src/components/plugins/InstalledList.tsx Outdated Show resolved Hide resolved
plugin-catalog/src/components/plugins/List.tsx Outdated Show resolved Hide resolved
plugin-catalog/src/components/plugins/List.tsx Outdated Show resolved Hide resolved
@joaquimrocha
Copy link
Contributor

I have now tried this:

  1. Stop the backend, to simulate issues.
  2. Try installing a plugin: it still shows the stop button briefly and proceeds to indicate it's installed 🤔

@yolossn
Copy link
Contributor Author

yolossn commented Jul 3, 2024

I have now tried this:

  1. Stop the backend, to simulate issues.
  2. Try installing a plugin: it still shows the stop button briefly and proceeds to indicate it's installed 🤔

The plugin installation isn't dependent on the backend. the backend is used only for the externalproxy to get plugin info from ArtifactHub.

@illume
Copy link
Contributor

illume commented Jul 3, 2024

The error I'm getting is coming from PluginManager.list inside InstallList.tsx So I guess it has something to do with plugins I have installed (or not installed).

Error loading Installed plugins: TypeError: Cannot read properties of undefined (reading 'list')

@yolossn yolossn force-pushed the plugin_store branch 2 times, most recently from 49b1e8e to 0ccfdfa Compare July 18, 2024 16:39
@illume
Copy link
Contributor

illume commented Jul 18, 2024

btw. The DCO check is failing, and probably those fixup commits should be applied.

Copy link
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

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

Noticed a lint issue, and a required dependency update.

plugin-catalog/package.json Outdated Show resolved Hide resolved
yolossn and others added 18 commits July 25, 2024 13:58
Plugin catalog is a plugin that helps
install and manage headlamp plugins
from artifacthub

Signed-off-by: yolossn <sannagaraj@microsoft.com>
…Card

Passing an object's members as props to a component is not a good practice.
So these changes instead pass the package itself.

Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
…tory

Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
Now instead of showing Learn More, we have the link for the repo in
the repo name.

Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
This patch adds links to the repo and org of the plugin and renames
some of the labels to be more descriptive to what they do.

Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
This goes to the whole plugin settings list instead of the actual
individual plugin settings until we have a way to do that more
easily.

Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
When one plugin was installed, they were higher than others.

Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
With npm run lint -- --fix.

Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
Copy link
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

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

👍

@joaquimrocha joaquimrocha merged commit b0c34ca into main Jul 25, 2024
9 checks passed
@joaquimrocha joaquimrocha deleted the plugin_store branch July 25, 2024 13:16
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