-
Notifications
You must be signed in to change notification settings - Fork 35
app-catalog: Support serviceproxy and support helm repos in the catalog #304
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?
app-catalog: Support serviceproxy and support helm repos in the catalog #304
Conversation
Hello, thanks for this. I hope to try it out and review in more detail soon. There's some type check issues being reported: npm run tsc https://github.com/headlamp-k8s/plugins/actions/runs/15910734305/job/45199330949?pr=304#step:6:12 ".": tsc-ing, :node_modules/.bin/tsc --noEmit:...
Error: src/api/charts.tsx(57,30): error TS2339: Property 'headers' does not exist on type 'Promise<any>'.
Error: src/components/charts/Details.tsx(92,51): error TS2339: Property 'icon' does not exist on type '{ name: string; description: string; logo_image_id: string; readme: string; app_version: string; maintainers: { name: string; email: string; }[]; home_url: string; package_id: string; version: string; }'.
Error: src/components/charts/List.tsx(194,25): error TS2339: Property 'entries' does not exist on type '{}'.
Error: src/components/charts/List.tsx(197,74): error TS2339: Property 'entries' does not exist on type '{}'.
Error: src/components/charts/List.tsx(199,25): error TS2339: Property 'packages' does not exist on type '{}'.
Error: src/components/charts/List.tsx(219,36): error TS2339: Property 'map' does not exist on type 'unknown'. Would you mind adding the signoff to the commit? This repo requires it. git commit --amend -s |
Please let us know if you're able to work on the fixes needed for this mentioned previously? If you're not able to, we could take this over for you. However, we'd need the commit signed off with:
|
ae85a79
to
7776b8c
Compare
@illume Could you please take a look and submit your feedback ? Thanks |
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 for those updates.
I marked all the conversations resolved where you'd made changes. Also I left a few notes.
A few questions:
- How can I test this manually?
- what is /service proxy
- I think instead of using global variables, the plugin configuration to use a helm repo instead of artifact hub can be passed into the components as properties.
variant: 'error', | ||
autoHideDuration: 5000, | ||
}); | ||
return; |
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 think this return is needed so when the fetch fails it doesn't continue on, because the code below uses that fetch response.
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.
@muraliinformal can you please check this one?
Oh. btw... Also please check if |
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.
Pull Request Overview
This PR adds support for securely fetching charts from both Vanilla Helm and Artifact Hub repositories using the /serviceproxy route, enabling dynamic catalog management in the app-catalog plugin.
- Added serviceproxy support for secure in-cluster chart access
- Enabled support for Helm repositories via index.yaml parsing
- Dynamically generated sidebars and routes based on catalog metadata
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
File | Description |
---|---|
app-catalog/src/index.tsx | Updated plugin initialization to dynamically register sidebars and routes based on catalog metadata |
app-catalog/src/helpers/catalog.ts | Added catalog listing functionality and component version mapping |
app-catalog/src/components/releases/EditorDialog.tsx | Fixed chart name handling for app-catalog helm repository |
app-catalog/src/components/charts/List.tsx | Enhanced chart list to support both Helm and ArtifactHub protocols with icon fetching |
app-catalog/src/components/charts/EditorDialog.tsx | Added support for vanilla Helm repository chart installation |
app-catalog/src/components/charts/Details.tsx | Added conditional icon rendering for different chart protocols |
app-catalog/src/api/charts.tsx | Implemented serviceproxy support and added chart icon fetching functionality |
app-catalog/src/api/catalogs.tsx | Added catalog management API with protocol support for Helm and ArtifactHub |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@illume I have addressed your comments, could you please take a look ? |
Thank you very much @muraliinformal I will do another review shortly. Seems there's quite a lot of type errors. Would you mind have a look? |
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.
Pull Request Overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
27a1aca
to
e377060
Compare
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 for all those changes.
I went through all of the conversations and resolved the ones done. There were a few still open. Also, I left a couple more questions.
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.
Pull Request Overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@muraliinformal theres a few open conversations. Can you please let me know what you think about each of them? Please see the commit guidelines https://headlamp.dev/docs/latest/contributing#2-follow-commit-guidelines Could you please squash your changes and rebase against main? If it makes sense to break it up into some smaller independent atomic commits, please do that? |
e721581
to
46820a2
Compare
Signed-off-by: Murali Annamneni <[email protected]>
…s review comments Signed-off-by: Murali Annamneni <[email protected]>
46820a2
to
3f9c810
Compare
@yolossn can you please review this too? |
@muraliinformal Do you have some steps to test this? I will run through this manually testing it, and ask another person to also help me manually test it. |
This PR adds support for securely fetching charts from both Vanilla Helm and Artifact Hub repositories using the /serviceproxy route.
Changes:
Related and required Headlamp backend PR: