-
Notifications
You must be signed in to change notification settings - Fork 278
Headlamp Plugins CLI: Bulk installations support #2974
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
base: main
Are you sure you want to change the base?
Conversation
plugins/headlamp-plugin/plugin-management/multi-plugin-management.js
Outdated
Show resolved
Hide resolved
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.
The src/
directory we use is based on typescript for better type safety, consistency and better documentation.
So, I would request you to please address this and change the manager
in ts as well to avoid inconsistency.
@@ -0,0 +1,125 @@ | |||
const path = require('path'); |
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.
the name of the folder plugin-management seems redundant. It can be something simpler 1 word like manager
or something like that.
Saw the previous files I think it should be fine to keep it in JS. |
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.
Some suggestions
plugins/headlamp-plugin/plugin-management/multi-plugin-management.js
Outdated
Show resolved
Hide resolved
plugins/headlamp-plugin/plugin-management/multi-plugin-management.js
Outdated
Show resolved
Hide resolved
properties: { | ||
name: { type: 'string' }, | ||
source: { type: 'string' }, | ||
version: { type: 'string' }, // Defaults to latest |
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 have validation for version format, also there is no constraints on plugin names (could have spaces/special chars).
Maybe something like
# for name
pattern: '^[a-z0-9-]+$' // Only allow lowercase, numbers, hyphens
# for version
pattern: '^\\d+\\.\\d+\\.\\d+' // Semver format
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.
For name, I've also allowed underscore (except at the start and end), because headlamp plugin names are using underscores somewhere IMO. e.g., https://artifacthub.io/packages/headlamp/test-123/ai_plugin.
So used ^[a-z0-9][a-z0-9-_]*[a-z0-9-]$
plugins/headlamp-plugin/plugin-management/multi-plugin-management.js
Outdated
Show resolved
Hide resolved
plugins/headlamp-plugin/plugin-management/multi-plugin-management.js
Outdated
Show resolved
Hide resolved
plugins/headlamp-plugin/plugin-management/multi-plugin-management.js
Outdated
Show resolved
Hide resolved
I've implemented the above suggestions, thanks for the initial review. Some additional things:
|
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-contributor-experience at kubernetes/community. /check-cla |
Signed-off-by: Faakhir30 <[email protected]>
Signed-off-by: Faakhir30 <[email protected]>
…ml config Signed-off-by: Faakhir30 <[email protected]>
Signed-off-by: Faakhir30 <[email protected]>
Signed-off-by: Faakhir30 <[email protected]>
Signed-off-by: Faakhir30 <[email protected]>
Signed-off-by: Faakhir30 <[email protected]>
Signed-off-by: Faakhir30 <[email protected]>
Signed-off-by: Faakhir30 <[email protected]>
Signed-off-by: Faakhir30 <[email protected]>
Signed-off-by: Faakhir30 <[email protected]>
37593d0
to
e6192e8
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Faakhir30 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…t.js in the file list
Signed-off-by: Faakhir30 <[email protected]>
Will support for Github and other hosting sites be added or is ArtifactHub the defacto right now? |
Refs #2787, first PR towards original propossed solution.
Scope and Tasks of this PR:
PluginManager
for installation, URL and version validations ...)The plugin-level config option was removed for now, as it might cause unnecessary confusion, though saved the TODO-based implementation that I removed in git stash, can be redone if required.
Testing:
individual Plugin with URL:
bin/headlamp-plugin.js install --source https://artifacthub.io/packages/headlamp/test-123/appcatalog_headlamp_plugin --folderName tmp
Bulk installations:
bin/headlamp-plugin.js install --config tmp/plugin-config.yml --folderName tmp
Make suretmp
directory is present.Unit Tests:
For
jest
based tests:f@f:~/w/oss/headlamp/plugins/headlamp-plugin$ npx jest plugin-management/*
Would refine commit messages at end(once reviews and changes required implemented) to avoid force pushing commits again n again.