validate-maintainers is too restrictive

[jeff-knurek]

trafficstars

Is this a request for help?:

maybe

Is this a BUG REPORT or FEATURE REQUEST? (choose one):

Could be a bug, but depends on documentation of helm.

What happened:

When enabling validate-maintainers, there's a check in pkg/chart/chart.go:

// ValidateMaintainers validates maintainers in the Chart.yaml file. Maintainer names must be valid accounts // (GitHub, Bitbucket, GitLab) names. Deprecated charts must not have maintainers.

And this means using a "name" that doesn't map to Github fails validation.

What you expected to happen:

The helm docs (https://helm.sh/docs/topics/charts/#the-chart-yaml-file) don't indicate that the maintainer name must be a valid VCS account name. I feel like this part of the validation should be optional (as I feel that is what is implied by the helm doc).

How to reproduce it (as minimally and precisely as possible):

Enable validate-maintainers in the lint. And test against a chart with the following config:

maintainers:
  - name: valid
    email: [email protected]
  - name: valid-too
    email: [email protected]

Interestingly, that's exactly what the unit test does, however valid-too isn't a valid github account... so this test should actually fail???

Anything else we need to know:

If it's agreed that the VCS check should be optional, and since this change would impact current usage, I'd be ok with it being enabled by default.

[RonaldCrb]

i have been playing with this on the ct docker image using these values literally.

maintainers:

• name: john doe email: [email protected] url: www.whatever.com

and the linting pass with no problems

[jeff-knurek]

hmmm... I tried with a new chart and it worked fine, but modified an existing chart that throws an error with the above example and I get:

Validating maintainers...
Error linting charts: Error processing charts
------------------------------------------------------------------------------------------------------------------------
 ✖︎ my-chart => (version: "4.1.10", path: "helm/my-chart") > Error validating maintainer 'john doe': 404 Not Found
------------------------------------------------------------------------------------------------------------------------

[jeff-knurek]

ok, I managed to get some commands to reproduce

docker run --rm -it --entrypoint=/bin/sh quay.io/helmpack/chart-testing:latest (where latest is v2.4.0)

git clone https://github.com/kubernetes/charts.git
cd charts/

# this should work because nothing has changed
ct lint --chart-dirs=stable 

# edit a file, bump the version, and add "john doe" maintainer
vi stable/anchore-engine/Chart.yaml

# this now fails
ct lint --chart-dirs=stable 

[jeff-knurek]

@RonaldCrb @unguiculus any thoughts on this? Should I work on a PR to disable this VCS name check?

[batazor]

I was faced with the same behavior

[unguiculus]

The whole point of this check is to make sure the accounts exist. You can disable the check if you don't want that.

[jeff-knurek]

The whole point of this check is to make sure the accounts exist.

And this is where myself, and at least @batazor disagree.

There's nothing in the helm documentation that says a maintainer has to be a valid VCS account. Any name & email should also be considered valid. Any name for that matter based on the documentation.

My opinion is that the current state of the check goes against the reason for the maintainers field, and disabling the check leaves us with the risk of lower quality charts. I mean, why would thee even be email and url fields if the [VCS] name is enough?

[unguiculus]

Referring to the Helm docs won't help here. This is chart-testing, not Helm. The reason this check is there is that we want to have it like this for the CI in the community charts repo.

Please disable the check if you don't need it. This does not necessarily leave you with the risk of lower quality charts. We also do validation of the Chart.yaml using Yamale which you can fine-tune to your needs. Here's the default config. Adjust it if needed: https://github.com/helm/chart-testing/blob/master/etc/chart_schema.yaml#L17-L20

[devth]

Seems like GitHub/Bitbucket/GitLab username should be a separate field. My name is not devth.

[jeff-knurek]

Seems like GitHub/Bitbucket/GitLab username should be a separate field

fully agree, though I think that would be a bigger change to happen in helm. But the idea behind such a change is why I'm advocating to make the vsc check optional; name != vsc username

---

ps: sorry, but I only know you as devth 😛

[taihen]

Facing the same issue. While it's a noble idea, in execution in team or organisation fails because of use true "Real Names" as a maintainer which does not map to VCS ;(

[bdashrad]

It would be nice if it supported github team names as well, since we often use those in charts

[mattfarina]

I might be able to add some context here.

There is the Helm charts repository. As a requirement of that repository, one must supply their GitHub username as the maintainers name. Chart Testing was born out of the charts repo CI system as others wanted to use the same features. Over time this was expanded to usernames in other systems (Gitlab, etc) per request.

The flag naming may be the issue. It's to validate the maintainers name is a valid username in a system (e.g., GitHub). It's useful for those repositories that have chosen to require it. This feature can be enabled or disabled.

There's a specific use case for it.

If you want to validate that the content of the maintainers field simply meets the schema you can use the schema validation capabilities of ct.

[jeff-knurek]

Thanks for the context @mattfarina

Maybe the issue is that the flag validate-maintainers is just a boolean, and limits checking what could otherwise be useful options.

For example, checking that maintainers simply meets the schema, doesn't check to make sure that a deprecated chart doesn't have a maintainer, nor does the schema check make sure that a non-deprecated chart does have a maintainer.

[TJM]

I have decided to set my name like:

maintainers:
  - name: TJM  # Tommy McNeely

... which makes this validator happy, and still puts useful information on the line for "humans" reading the file. I couldn't get --validate-maintainers false to work (shrug).

[caleblloyd]

@TJM

I couldn't get --validate-maintainers false to work (shrug).

I couldn't either but I discovered that using an equals sign --validate-maintainers=false does work. Apparently this is a limitation of the CLI library, cobra

[andy108369]

I couldn't get --validate-maintainers false to work (shrug).

It works when you add = sign there, like this:

--validate-maintainers=false

[gtirloni]

If this check was created to satisfy a requirement of the community charts repository, why not enable it explicitly there and not for all ct users everywhere?

I was trying ct today and got surprised by this 404 error too. We do not use GitHub names in the name field like that and the error message is cryptic at best.

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[github-actions[bot]]

This issue was closed because it has been stalled for 5 days with no activity.