-
Notifications
You must be signed in to change notification settings - Fork 7k
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
populate chart metadata with a repoURL [hip-0018] #11378
base: main
Are you sure you want to change the base?
Conversation
a4c634c
to
7326cca
Compare
if client.ChartPathOptions.RepoURL != "" { | ||
chartRequested.Metadata.RepoURL = client.ChartPathOptions.RepoURL | ||
} else { | ||
chartRequested.Metadata.RepoURL = "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.
If the chart did not originate from a repository, recommend omitting inserting path
into the RepoURL
field
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.
This was discussed during the HIP process, and ultimately decided to use a placeholder string. I'm happy to do whatever here, but wanted to make sure I followed the original HIP - https://github.com/helm/community/blob/main/hips/hip-0018.md#specification
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'd almost prefer the actual path. It's a good hint if I forgot where I installed from, maybe as a file:// url?
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.
That could work, and I'm open to whatever y'all prefer as the maintainers. For most of the planned use-cases I have seen, the path for local charts is far less important than remote repository URLs. That being said, I can see the value of having the filepath as well.
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.
local path it's nice feature, because in big teams we can see who touched chart at last time :)
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.
@sudermanjr Thanks for the PR. In addition to what @sabre1041 asked for, could you possibly add some simple tests for this?
7326cca
to
0365071
Compare
Signed-off-by: Luke Reed <luke@lreed.net> support OCI registries as well Signed-off-by: Luke Reed <luke@lreed.net> revert changes to DownloadTo function and implement using the ChartDownloader object instead Signed-off-by: Luke Reed <luke@lreed.net> make sure update will still update repoURL Signed-off-by: Luke Reed <luke@lreed.net> Signed-off-by: Andy Suderman <andy@suderman.dev> Set the repository URL to path if a URL is not found Signed-off-by: Andy Suderman <andy@suderman.dev> Fixes Signed-off-by: Andy Suderman <andy@suderman.dev>
0365071
to
799c1b6
Compare
@mattfarina I addressed the other question, and I'm looking at adding some tests, but I'm struggling a bit with the right way to approach that. The pkg/metadata changes are covered by existing tests. The tests in the cmd package seem to deal mostly with CLI output, and I don't see anything verifying chart metadata in the release. Chart_downloader seems to be a candidate for a simple test, but everything I come up with feels a bit contrived, and not valuable as an actual test. Did you have any specific thoughts about the approach for these tests? |
guys, here is needed some help or not? how we can accelerate that PR? |
What this PR does / why we need it:
Implements https://github.com/helm/community/blob/main/hips/hip-0018.md
Fixes: #4256
Signed-off-by: Luke Reed luke@lreed.net
Signed-off-by: Andy Suderman andy@suderman.dev
Special notes for your reviewer:
rebased off of, and supersedes (with permission from original author): #10369
If applicable:
Output when using OCI registry
Output when installing from a locally pulled tarball or a path
Output when pulling from a standard helm repository