HIP: Add Exclude File Option to Helm Lint Command #353

danilo-patrucco · 2024-07-03T15:09:36Z

Added HIP 0019 for the development of the lint ignore feature in helm binaries
refer to issue in helm repository #13133

Signed-off-by: Danilo Patrucco <[email protected]> Signed-off-by: danilo-patrucco <[email protected]> Signed-off-by: Danilo Patrucco <[email protected]> Signed-off-by: Danilo Patrucco <[email protected]>

Add Daniel Pritchett due to his participation in the feature development Signed-off-by: danilo patrucco <[email protected]>

See HIP-0019 proposal at helm/community: helm/community#353 Co-authored-by: Danilo Patrucco <[email protected]> Signed-off-by: Daniel J. Pritchett <[email protected]>

See HIP-0019 proposal at helm/community: helm/community#353 Co-Authored-By: Danilo Patrucco <[email protected]> Signed-off-by: Daniel J. Pritchett <[email protected]>

See HIP-0019 proposal at helm/community: helm/community#353 Co-authored-by: Danilo Patrucco <[email protected]> Signed-off-by: Daniel J. Pritchett <[email protected]>

- adds ignorer usage to cmd/helm/lint.go - adds ignorer usage to pkg/action/lint.go and its lint_test.go See HIP-0019 proposal at helm/community: helm/community#353 Co-authored-by: Danilo Patrucco <[email protected]> Signed-off-by: Daniel J. Pritchett <[email protected]>

hips/hip-0019.md

gjenkins8 · 2024-09-12T21:02:33Z

hips/hip-0019.md

+The `.helmlintignore` file will support simple patterns to match files and directories for easy exclusion management. The `--lint-ignore-file` flag enhances this by allowing centralized management of lint exclusions in complex projects. 
+## Specification 
+### `.helmlintignore` File Format 
+The `.helmlintignore` file allows chart developers to specify filenames or glob patterns to exclude from linting. The format is straightforward, with one pattern per line, similar to `.gitignore` files. 


I think a yaml format that can encode different types of rules/options for linting would be much more preferable than a custom / one-file-per-line format (that is limited to just that).

I also think that all/nothing for lint ignoring (per-file) is too coarse for the general case. Especially since many lint rules pertain (in Helm) to e.g. Chart.yaml (ref). Likely it normally would be better to exclude certain rules for Chart specific files (Chart.yaml, values.yaml (default values), etc), and paths for templates, etc. See https://github.com/helm/helm/tree/main/pkg/lint/rules for current Helm linting rules.

Also take e.g. https://yamllint.readthedocs.io/en/stable/configuration.html#configuration or https://ansible.readthedocs.io/projects/lint/configuring/#ansible-lint-configuration as inspirations/examples for what might go into a lint configuration (of course not every there will apply to Helm).

@GeorgeJ
I have a question about this comment:
I also think that all/nothing for lint ignoring (per-file) is too course for the general case. Especially since many lint rules pertain (in Helm) to e.g. Chart.yaml (ref). Likely it normally would be better to exclude certain rules for Chart specific files (Chart.yaml, values.yaml (default values), etc), and paths for templates, etc. See https://github.com/helm/helm/tree/main/pkg/lint/rules for current Helm linting rules.
In response this discussion about the broad approach to lint ignoring, especially for key Helm config files like Chart.yaml, I want to discuss the current errors handling from the PR that we submitted. Right now we remove errors rather than skipping them altogether
Here’s the gist:
Error removal: We let all errors get logged as usual but we remove them from the final error list that users see. This keeps the linting thorough but the user interface clean.
Debugging Option: If someone needs to dive deep or is troubleshooting, they can use the --debug flag to see everything, removed errors included.
Post-Processing Advantages: Handling the error list after it’s fully formed means we clean things up at the end. This makes managing exceptions way easier and ensures we’re not tossing useful feedback too early.
I see a few pluses here:
Flexible Error Reports: You get a neat, easy-to-digest error report by default but can switch to a detailed view when needed.
Better Troubleshooting: Keeping all the linting info till the end helps us understand what we're filtering out and why, making it easier to adjust if something's not quite right.
What you are suggesting would be more aligned with something like this :we would need to change the structure of the lintError variable to something like this:

type LintError struct { // Err is the original linter error Err error // ChartPath is the string path to the chart that generated this linter error ChartPath string // TemplatePath is nullable because some errors are chart-level while others are template-specific TemplatePath *string }

In this way we would be able to better review and analyze the errors and even list them in the output, giving an idea of how many errors are ignored, but that would require a pretty important lift, because it would change multiple files/potentially structures/functions

Reply from @gjenkins8

Morning, I don't disagree with the above (the "post-filtering" approach). I took a look at the reworked HIP, it looks good to me! Thanks for the updates
If the idea of specific rule based exclusions is too much for the moment, I think fine to pivot and just support what you have described (the fileMask and errorMask "directives")
I would suggest to small updates the HIP with some scope details:
only file / error based exclusions (masks) considered
rule-based exclusions (& configuration) considered difficult to implement, and skipped for the moment
My major/main goal was to make sure e.g. like yaml lint that the lint configuration can be extended in the future. I think it can with the current format (but I will put some comments on the format on the HIP)

hips/hip-0019.md

Signed-off-by: Danilo Patrucco <[email protected]>

gjenkins8 · 2024-09-22T14:54:47Z

hips/hip-0019.md

+#### Example
+```
+# .helmlintconfig.yaml file example
+lintIgnore:


perhaps a format more like (bikeshedding hard here):

# .helmlintconfig.yaml file example pathIgnore: - "migrations/templates/job.yaml" - "gitlab/templates/shared-secrets/self-signed-cert-job.yml" - "gitlab/templates/*.yaml" errorIgnore: - "chart metadata is missing these dependencies*" - "<include 'fullname' .>"

I modified this section a little, but kept it similar to our suggestion because an error (the same error specifically) can occur in multiple charts / sub-charts, and connecting it to a file path would be beneficial (sorry for the bikeshedding here)

gjenkins8 · 2024-09-22T14:54:50Z

hips/hip-0019.md

+- Developing an effective and user-friendly pattern syntax for the `.helmlintconfig.yaml` file.
+- Considering the potential for in-file annotations to temporarily exclude specific chart sections from linting.
+
+## Rejected Ideas


Providing rule specific excludes and configurations, and composing configuration from "default" configuration e.g. similar to https://yamllint.readthedocs.io/en/stable/configuration.html#extending-the-default-configuration

Helm today doesn't "name" its lint rules, and changing the code/structure to support the above can be extended later

Done, let me know if it's ok

oops, sorry, I mean we should add these to the "rejected ideas":

Providing rule specific excludes (Helm today doesn't "name" its lint rules)

composing configuration from "default" configuration (e.g. similar to https://yamllint.readthedocs.io/en/stable/configuration.html#extending-the-default-configuration)

gjenkins8 · 2024-09-22T14:56:01Z

hips/hip-0019.md

+
+## Specification
+### `.helmlintconfig.yaml` File Format
+The `.helmlintconfig.yaml` file enables chart developers to specify filenames or glob patterns to exclude from linting. The format is YAML, with patterns specified under a `lintIgnore` key, making future extensions for additional configurations feasible.


glob patterns for both file paths and error strings

Signed-off-by: Danilo Patrucco <[email protected]>

dpritchett · 2024-09-24T14:26:24Z

hips/hip-0019.md

+---
+
+## Abstract
+This proposal introduces enhancements to the `helm lint` command, allowing the exclusion of specific files or patterns through a `.helmlintconfig.yaml` file, analogous to `.gitignore` or `.helmignore`. Additionally, a `--lint-config-file` flag will be added to specify alternative locations for the `.helmlintconfig.yaml` file. This feature is particularly useful in projects with multiple sub-charts. An environment variable `HELM_LINT_CONFIG_FILE` will also be introduced to mirror the functionality of the flag.


Can we reword analogous to .gitignore or .helmignore now that we're targeting a general-purpose config file format?

dpritchett · 2024-09-24T14:28:38Z

hips/hip-0019.md

+This proposal introduces enhancements to the `helm lint` command, allowing the exclusion of specific files or patterns through a `.helmlintconfig.yaml` file, analogous to `.gitignore` or `.helmignore`. Additionally, a `--lint-config-file` flag will be added to specify alternative locations for the `.helmlintconfig.yaml` file. This feature is particularly useful in projects with multiple sub-charts. An environment variable `HELM_LINT_CONFIG_FILE` will also be introduced to mirror the functionality of the flag.
+
+## Motivation
+In large Helm charts, certain files or configurations may not conform to standard linting rules but are accepted by the maintainers. The current `helm lint` process evaluates all files within a chart, often leading to irrelevant warnings or errors. An option to exclude specific files and the ability to specify config file locations would streamline the linting process.


Potential phrasing: this can put helm users in situations where imported subcharts are flagged with low-priority lint errors that they'd prefer to suppress

dpritchett · 2024-09-24T14:35:22Z

hips/hip-0019.md

+```
+
+### Command Behavior
+When `helm lint` is executed, it checks for the presence of a `.helmlintconfig.yaml` file in the chart directory or at a location specified by the `--lint-config-file` flag. The command then parses this file to exclude any files or directories matching the patterns specified, including specific error lines.


or wherever the env var says to look

dpritchett · 2024-09-24T14:36:23Z

hips/hip-0019.md

+Documentation for the `.helmlintconfig.yaml` file and the `--lint-config-file` flag will be integrated into the official Helm documentation under the `helm lint` section. Detailed examples and common patterns will be provided to assist new users.
+
+## Reference Implementation
+PR #[13257](https://github.com/helm/helm/pull/13257) created in the Helm/helm repository in gitlab 


n.b. this PR will need some significant changes to catch up with the current design as described here in HIP-0019

this i will leave untouched if it's ok with you, since we put the PR in draft

dpritchett · 2024-09-24T14:38:18Z

hips/hip-0019.md

+PR #[13257](https://github.com/helm/helm/pull/13257) created in the Helm/helm repository in gitlab 
+
+## Open Issues
+- Developing an Effective and User-Friendly Pattern Syntax: We are considering the design of a flexible and intuitive pattern syntax for the `.helmlintconfig.yaml` file. This syntax would ideally support rule-specific configurations and exclusions, allowing users to finely control lint behaviors on a per-rule basis.


this piece is not self-evident to me, can we add a hypothetical such as "errorCode: H019 would allow for across-the board suppression of errors by error code. We are deferring this for now because it will require a refactor of all 50+ existing helm lint cases."

dpritchett · 2024-09-24T14:41:15Z

hips/hip-0019.md

+## Open Issues
+- Developing an Effective and User-Friendly Pattern Syntax: We are considering the design of a flexible and intuitive pattern syntax for the `.helmlintconfig.yaml` file. This syntax would ideally support rule-specific configurations and exclusions, allowing users to finely control lint behaviors on a per-rule basis.
+- In-File Annotations for Excluding Chart Sections: Exploring the possibility of in-file annotations that would let developers temporarily exclude specific sections of a chart from linting. This feature would provide granular control directly within Helm chart files.
+- Rule Naming and Configurations: Currently, Helm does not support named lint rules. Introducing named rules is a foundational step that would enable us to implement more sophisticated configuration features, such as rule-specific exclusions and settings. This would also allow users to extend or override a "default" configuration set, similar to practices seen in tools like [yamllint](https://yamllint.readthedocs.io/en/stable/configuration.html#extending-the-default-configuration). Naming rules would facilitate clearer and more maintainable configurations, laying the groundwork for future enhancements.


Looks like bullet points 1 and 3 here might be overlapping, maybe we could consolidate?

dpritchett · 2024-09-24T14:42:12Z

hips/hip-0019.md

+- Developing an Effective and User-Friendly Pattern Syntax: We are considering the design of a flexible and intuitive pattern syntax for the `.helmlintconfig.yaml` file. This syntax would ideally support rule-specific configurations and exclusions, allowing users to finely control lint behaviors on a per-rule basis.
+- In-File Annotations for Excluding Chart Sections: Exploring the possibility of in-file annotations that would let developers temporarily exclude specific sections of a chart from linting. This feature would provide granular control directly within Helm chart files.
+- Rule Naming and Configurations: Currently, Helm does not support named lint rules. Introducing named rules is a foundational step that would enable us to implement more sophisticated configuration features, such as rule-specific exclusions and settings. This would also allow users to extend or override a "default" configuration set, similar to practices seen in tools like [yamllint](https://yamllint.readthedocs.io/en/stable/configuration.html#extending-the-default-configuration). Naming rules would facilitate clearer and more maintainable configurations, laying the groundwork for future enhancements.
+- Future Enhancements and Extensibility: The proposed changes to the Helm linting system will likely require significant modifications to the existing codebase. As such, these enhancements could be implemented in phases, starting with foundational features like rule naming and extending to more complex configurations. This phased approach will help ensure stability and usability while expanding the linting capabilities of Helm.


The proposed changes to the Helm linting system will likely require significant modifications

The basic design in this HIP won't dramatically alter the helm codebase, but pretty much all of the stretch goals here in "open issues" would require some broader helm refactors.

TerryHowe · 2024-09-24T16:56:12Z

hips/hip-0019.md

+---
+
+## Abstract
+This proposal introduces enhancements to the `helm lint` command, allowing the exclusion of specific files or patterns through a `.helmlintconfig.yaml` file, analogous to `.gitignore` or `.helmignore`. Additionally, a `--lint-config-file` flag will be added to specify alternative locations for the `.helmlintconfig.yaml` file. This feature is particularly useful in projects with multiple sub-charts. An environment variable `HELM_LINT_CONFIG_FILE` will also be introduced to mirror the functionality of the flag.


Suggested change

This proposal introduces enhancements to the `helm lint` command, allowing the exclusion of specific files or patterns through a `.helmlintconfig.yaml` file, analogous to `.gitignore` or `.helmignore`. Additionally, a `--lint-config-file` flag will be added to specify alternative locations for the `.helmlintconfig.yaml` file. This feature is particularly useful in projects with multiple sub-charts. An environment variable `HELM_LINT_CONFIG_FILE` will also be introduced to mirror the functionality of the flag.

This proposal introduces enhancements to the `helm lint` command, allowing the exclusion of specific files or patterns through a `.helmlintignore` file, analogous to `.gitignore` or `.helmignore`. Additionally, a `--lint-config-file` flag will be added to specify alternative locations for the `.helmlintignore` file. This feature is particularly useful in projects with multiple sub-charts. An environment variable `HELM_LINT_IGNORE_FILE` will also be introduced to mirror the functionality of the flag.

Hi Terry, Following our discussion with George, we've changed the .helmlintignore file to .helmlintconfig.yaml. The reason for this change is that the new config file will be in YAML format, and it will not only allow us to ignore specific linting errors but in the future we will also add more options/rules for linting. This is similar to the hadolint.yaml file that supports ignoring errors and setting up specific linting rules and options.

Also, We did put the PR for helm/helm back in Draft, since we are waiting to have a finalized version of the HIP to continue the development work on the helm repo. Please let me know If you have any additional reservation, and thank you for the review

Signed-off-by: Danilo Patrucco <[email protected]>

gjenkins8

LGTMe

Final touchup would be adding the rejected ideas:
https://github.com/helm/community/pull/353/files#r1788505454

Perhaps some other eyes on the file format. Since this is the main user facing aspect, and important to get the UX right.

Signed-off-by: Danilo Patrucco <[email protected]>

danilo-patrucco · 2024-10-10T17:43:29Z

@gjenkins8 fixed the changes

danilo-patrucco · 2024-10-16T16:17:13Z

@mattfarina Hi Matt, would you be able to review this PR in addition to George and give us also your opinion on the file format ?

pull-request-size bot added the size/M label Jul 3, 2024

danilo-patrucco force-pushed the helm_ignore_HIP branch from 3a6938e to 035b33d Compare July 3, 2024 15:11

add hip-0019

1385741

Signed-off-by: Danilo Patrucco <[email protected]> Signed-off-by: danilo-patrucco <[email protected]> Signed-off-by: Danilo Patrucco <[email protected]> Signed-off-by: Danilo Patrucco <[email protected]>

danilo-patrucco force-pushed the helm_ignore_HIP branch from 035b33d to 1385741 Compare July 3, 2024 15:34

danilo-patrucco changed the title ~~add hip-0019~~ HIP: Add Exclude File Option to Helm Lint Command Jul 3, 2024

Update hip-0019.md

b8e3b42

Add Daniel Pritchett due to his participation in the feature development Signed-off-by: danilo patrucco <[email protected]>

dpritchett mentioned this pull request Aug 16, 2024

HIP-0019 adds .helmlintignore functionality to pkg/lint helm/helm#13257

Draft

gjenkins8 reviewed Sep 12, 2024

View reviewed changes

change some wording and add more information

168c2e5

Signed-off-by: Danilo Patrucco <[email protected]>

gjenkins8 reviewed Sep 22, 2024

View reviewed changes

modifying the file additionaly after feedback

00809c1

Signed-off-by: Danilo Patrucco <[email protected]>

dpritchett reviewed Sep 24, 2024

View reviewed changes

TerryHowe reviewed Sep 24, 2024

View reviewed changes

danilo-patrucco added 2 commits September 24, 2024 14:08

initial changes as suggested by Daniel Pritchett

23e2488

Signed-off-by: Danilo Patrucco <[email protected]>

include some more changes

534e367

Signed-off-by: Danilo Patrucco <[email protected]>

danilo-patrucco requested review from TerryHowe, gjenkins8 and dpritchett September 24, 2024 18:55

gjenkins8 reviewed Oct 5, 2024

View reviewed changes

fix some changes that were supposed to go in the rejected ideas.

4e07227

Signed-off-by: Danilo Patrucco <[email protected]>

danilo-patrucco force-pushed the helm_ignore_HIP branch from 65ba1ba to 4e07227 Compare October 10, 2024 17:42

danilo-patrucco requested a review from gjenkins8 October 29, 2024 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HIP: Add Exclude File Option to Helm Lint Command #353

HIP: Add Exclude File Option to Helm Lint Command #353

danilo-patrucco commented Jul 3, 2024

gjenkins8 Sep 12, 2024 •

edited

Loading

danilo-patrucco Sep 23, 2024 •

edited

Loading

danilo-patrucco Sep 23, 2024

gjenkins8 Sep 22, 2024

danilo-patrucco Sep 23, 2024 •

edited

Loading

gjenkins8 Sep 22, 2024

danilo-patrucco Sep 23, 2024

gjenkins8 Oct 5, 2024

gjenkins8 Sep 22, 2024

danilo-patrucco Sep 23, 2024

dpritchett Sep 24, 2024

danilo-patrucco Sep 24, 2024

dpritchett Sep 24, 2024

danilo-patrucco Sep 24, 2024

dpritchett Sep 24, 2024

danilo-patrucco Sep 24, 2024

dpritchett Sep 24, 2024

danilo-patrucco Sep 24, 2024

dpritchett Sep 24, 2024

danilo-patrucco Sep 24, 2024

dpritchett Sep 24, 2024

danilo-patrucco Sep 24, 2024

dpritchett Sep 24, 2024

danilo-patrucco Sep 24, 2024

TerryHowe Sep 24, 2024

danilo-patrucco Sep 24, 2024

danilo-patrucco Sep 24, 2024 •

edited

Loading

gjenkins8 left a comment

danilo-patrucco commented Oct 10, 2024

danilo-patrucco commented Oct 16, 2024

HIP: Add Exclude File Option to Helm Lint Command #353

Are you sure you want to change the base?

HIP: Add Exclude File Option to Helm Lint Command #353

Conversation

danilo-patrucco commented Jul 3, 2024

gjenkins8 Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

danilo-patrucco Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danilo-patrucco Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danilo-patrucco Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

gjenkins8 left a comment

Choose a reason for hiding this comment

danilo-patrucco commented Oct 10, 2024

danilo-patrucco commented Oct 16, 2024

gjenkins8 Sep 12, 2024 •

edited

Loading

danilo-patrucco Sep 23, 2024 •

edited

Loading

danilo-patrucco Sep 23, 2024 •

edited

Loading

danilo-patrucco Sep 24, 2024 •

edited

Loading