-
Notifications
You must be signed in to change notification settings - Fork 707
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
enhances the ValidateAppRepository() code to valid OCI Repository type for Helm-Charts and gracefully report errors. #3605
Conversation
Signed-off-by: “satya-dillikar” <“satya-dillikar@gmail.com”>
Signed-off-by: “satya-dillikar” <“satya-dillikar@gmail.com”>
Signed-off-by: “satya-dillikar” <“satya-dillikar@gmail.com”>
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.
Great start @satya-dillikar ! The code that you've written is working, though there are a few things to check (I've left a bunch of info, given that you're new to go, let me know if it's too much).
There's two other things though stopping this from being landable:
- The tests you've added are disabled and can't work currently (since they won't use a fake http client). I've left some notes there about how you should instead modify the existing tests, and
- You've not yet deleted the existing code that will no longer be used (for eg.
getRequests
)
Let me know how you go with the testing and I'll check back.
Thanks!
Signed-off-by: “satya-dillikar” <“satya.dillikar@gmail.com”>
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 @satya-dillikar . A couple of small trivial comments about the code, but regarding the tests, it looks like you've been forced to remove some of the functionality that was being tested as a result of trying to join the two sets of tests into one. I've tried to further detail what I meant in my previous suggestion for the tests. Please have a go, but let me know if it's too hard to follow and I'll instead create a PR showing you what I mean.
Signed-off-by: “satya-dillikar” <“satya.dillikar@gmail.com”>
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 Satya. The tests look better now. As I mentioned on slack, I think we still need to do some work in the tests (the first set can be simplified and the second set need to be extended to also test the HelmOCIValidator), but I'm happy to do that in a separate PR as it'll be easier to show what I mean there.
For now, I'm +1 to land this once you confirm that it also works for charts uploaded with Helm 3.6 clients (or in particular, 3.5.4, since that's the version we're using). I suspect we may need to modify it to work since helm 3.7 changed the format slightly, which is probably the cause of the bug that you created.
If it doesn't work for a chart uploaded to an oci repo with a helm client < 3.7, then I suspect what we will want to do is ensure our code works with the pre 3.7 version (which will mean it fails for the 3.7+ version) so that kubeapps can deploy the chart (using its 3.5.4 client). We can then land this PR. After that we'll need to chat with the content team and find out what helm client is currently used to upload bitnami charts to OCI repos (for TAC/VAC) and/or when they plan to update to a helm 3.7 client, as we'll want to update around the same time.
I have packaged two helm charts (podinfo and wordpress) using helm 3.5.4 & pushed to internal harbor (https://harbor-repo.vmware.com/)
|
Excellent, thanks Satya! Landing! |
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.
Great work Satya
Description of the change
This PR enhances the ValidateAppRepository() code to valid OCI Repository type for Helm-Charts and gracefully report errors.
Benefits
User-visible error on UX for non-helm chart OCI Repositories
See screenshot
Possible drawbacks
Not known
Applicable issues
Additional information
N/A