feat(helm): consolidate validation warnings

[rcousineau-xandr]

Closes #9407

What this PR does / why we need it:

when doing the following commands:

• helm repo add
• helm repo update
• helm search repo

during validation of repository, invalid charts are counted and a single warning is printed:

skipped loading 2964 invalid chart entries from www.helm.repo

also, new helm repo update --show-all-warnings flag that shows a warning for each invalid chart

Special notes for your reviewer:

If applicable:

• [ ] this PR contains documentation
• [ ] this PR contains unit tests
• [ ] this PR has been tested for backwards compatibility

[rcousineau-xandr]

@bacongobbler here's an alt approach that might be better than https://github.com/helm/helm/pull/9604

[rcousineau-xandr]

@bacongobbler have you had a chance to look at this?

[pierluigilenoci]

@rcousineau-xandr maybe you should do a PR rebase.

@adamreese @bacongobbler @hickeyma @jdolitsky @marckhouzam @mattfarina @prydonius @SlickNik @technosophos

would anyone be so kind and helpful to take a look at this PR?

[schollii]

Over 2 months since @pierluigilenoci, is there any hope to get this approved, is there anything someone like me can do to help?

[pierluigilenoci]

@schollii I'm not a maintainer of this repo so I can't do anything more than tag all the maintainers. 😉

[schollii]

@pierluigilenoci I know, that's why I referenced your post, not much more we can do about this PR but flag people. I'm sure these guys are super busy of course.

cc @bacongobbler @mattfarina @technosophos

[bacongobbler]

Wayyyyyy too busy with other work to review this right now. I'll throw this onto the 3.7.0 milestone for reiew with the hopes that someone else might find the time to review this.

[bacongobbler]

One quick note: I am concerned that this masks what chart entries are masked. The user has no idea whether any chart entries they are attempting to pull in are part of that dataset or not.

skipped loading 2964 invalid chart entries from www.helm.repo

How am I supposed to know if the chart I'm pulling in with helm repo update is invalid?

[rcousineau-xandr]

if one needs the individual invalid chart names, the current behavior is preserved with

helm repo update --show-all-warnings

[schollii]

@bacongobbler Good point about the charts being pulled in. How about the flag only applies to charts that are not pulled in? So if in a helm install you are pulling in 3 charts, only those 3 would get warnings, regardless of the --show-all-warnings setting. Then if you really want all warnings even for charts that are not being used, use the flag.

I suggest also that the flag be more specific, like --warn-all-invalid-charts-in-repo. This is a bit verbose but it is clear what it is for, and if there are ever more types of warnings that should come to life, this will not interfere.

@rcousineau-xandr these should be small mods, if you don't have time I could try (based on responses to above suggestions).

[schollii]

Well it can't be "enumerate" unless each error has a number. Also I think it needs to be specific to index (although technically it is not the index that is incorrect, it is the chart version, but it's a gray area). So how about --show-index-validation-errors, or just --show-index-errors`?

[rcousineau-xandr]

Well it can't be "enumerate" unless each error has a number.

e·nu·mer·ate /əˈn(y)o͞oməˌrāt/ mention (a number of things) one by one. "there is not space to enumerate all his works"

Also I think it needs to be specific to index (although technically it is not the index that is incorrect, it is the chart version, but it's a gray area). So how about --show-index-validation-errors, or just --show-index-errors`?

It happens while indexing the repo, it is not validation of the index that fails.

--show-repo-validation-errors ?

[schollii]

I've been using Python for too long I guess ;)

I vote for --show-repo-validation-errors

[schollii]

@bacongobbler so IINM based on what we found the consolidated warning will make it clear that the final error about chart not downloaded is due to bad chart. If the chart downloads, then it was not one of the bad ones, and most users will not care about the details. Those that do, can re-run with the flag --show-repo-validation-errors.

[pierluigilenoci]

@schollii maybe make sense to resolve conflicts. I still have the secret hope that this PR will come to an end. 😜

[pierluigilenoci]

@schollii could you please take a look?

[rcousineau-xandr]

@schollii okay I've added a test

[schollii]

@rcousineau-xandr i will build and run. But I'm trying to remember how to reproduce this, do you remember how the invalid charts ended up in the helm registry? was it the public registry or a private registry in S3?

[rcousineau-xandr]

@schollii in my company's private helm repository on an instance of artifactory we have thousands of charts that were published prior to the helm 3.5.2 update that added the requirement that the version be semver compliant. so using a helm 3.5.2+ client and doing helm repo update, we get thousands of messages printed to the console, each one stating that an individual chart's version is invalid.

[jdmaguire]

To add some more context @schollii, before 3.5.2, helm could package and push helm charts with invalid semvers. This was considered an exploitation of a bug that was fixed in 3.5.2.

To get an invalid helm chart packaged/pushed, you either need to use an old helm version & helm push or say curl to push directly to a helm repository that doesn't do validation on incoming pushes.

It is quite common to accidentally not have a valid semver and hence a lot of people using pre-3.5.2 Helm could have been generating these invalid versions for years without knowing.

[schollii]

Yeah that makes sense. I no longer use registry, much prefer to install charts straight from git repo. So I'll have to pass on that part of the PR, but sounds like you have a good way to confirm it works at command line.

So good job @rcousineau and thanks for taking the time.

So now we just need official approval.

[pierluigilenoci]

@mattfarina are there any chances of seeing this PR merged?

[justinmchase]

Please, someone from the team drive this forward! Its a daily source of annoyance for me and trying to script around it requires so much added complexity. There is no reason at all this should be taking this much time or brain power. Just stop emitting the dang useless warnings please.

[sp-ludovic-ivain]

would love to have this PR merged too 🥺 🙏

[pierluigilenoci]

@adamreese @bacongobbler @hickeyma @jdolitsky @marckhouzam @mattfarina @sabre1041 @scottrigby @SlickNik @technosophos

Can any of you help out?

[justinmchase]

[pierluigilenoci]

@schollii could you please take a look?

[schollii]

@pierluigilenoci I gave my ok Apr 9 above, and AFAICT no commits since so I don't know what else to do (I do not have approval rights). If you have idea let me know.

[jdmaguire]

Looking at the comment history, "we" have pinged every maintainer a few times. I guess they're all busy or haven't seen the pings (understandable when there are 250+ open PRs in this repo). I'm not particularly sure what we could politely do to get someone's attention.

[schollii]

Indeed. Adding maintainers might be the only practical solution. I wonder what it takes to be a maintainer. I would love to but unfortunately I don't have the time. I don't even have to to look after my own projects! :confounded: