Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

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

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1385741
add hip-0019
danilo-patrucco-sev1tech Jul 3, 2024
b8e3b42
Update hip-0019.md
danilo-patrucco Aug 13, 2024
168c2e5
change some wording and add more information
danilo-patrucco Sep 19, 2024
00809c1
modifying the file additionaly after feedback
danilo-patrucco Sep 23, 2024
23e2488
initial changes as suggested by Daniel Pritchett
danilo-patrucco Sep 24, 2024
534e367
include some more changes
danilo-patrucco Sep 24, 2024
4e07227
fix some changes that were supposed to go in the rejected ideas.
danilo-patrucco Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions hips/hip-0019.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
---
hip: 0019
title: "Add Exclude File Option to Helm Lint Command"
authors: Danilo Patrucco, Daniel Pritchett
created: "2024-07-03"
type: "feature"
status: "draft"
---

## Abstract
This proposal suggests enhancing the `helm lint` command to exclude specific files or patterns from linting through a `.helmlintignore` file, similar to `.gitignore` or `.helmignore`. Additionally, a `--lint-ignore-file` flag will be introduced to specify alternative locations for the `.helmlintignore` file, facilitating its use in projects with multiple sub-charts.
danilo-patrucco marked this conversation as resolved.
Show resolved Hide resolved
danilo-patrucco marked this conversation as resolved.
Show resolved Hide resolved
## Motivation
In large Helm charts, certain files or configurations may deviate from standard linting rules but are accepted by the maintainers. Currently, `helm lint` evaluates all files within a chart, leading to irrelevant warnings or errors. An exclusion option and the ability to specify ignore file locations would allow developers to better manage lint results.
## Rationale
This proposal aims to give developers and integrators greater control over the linting process by allowing exclusions for:
- Files imported or managed by third parties that are not directly editable.
- Files that may not conform to static linting rules.
- Files containing intentional deviations from standard practices due to specific deployment scenarios.
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
danilo-patrucco marked this conversation as resolved.
Show resolved Hide resolved
### `.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.
Copy link
Contributor

@gjenkins8 gjenkins8 Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Author

@danilo-patrucco danilo-patrucco Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

#### Example
```
# .helmlintignore file example
template/test/test.sh # Exclude this file from linting
template/test/test.yaml {{template "fullname" .}} # Exclude this specific line from findings in this specific file (error is still detected but no output is generated)
```
Patterns can be a simple `path/to/my/file` to exclude a whole file, or `path/to/my/file line.triggering.error` to ignore specific error-triggering lines in the error output.
### Command Behavior
When `helm lint` is executed, it will first check for the presence of a `.helmlintignore` file in the chart directory or at a specified location using the `--lint-ignore-file` flag. This allows the command to parse the file and exclude any files or directories matching the patterns. Additionally, if a file contains a specific error line that matches a pattern, that error will not be displayed.
## Backwards Compatibility
This proposal introduces no breaking changes. Charts without a `.helmlintignore` file or without using the `--lint-ignore-file` flag will function as they currently do.
## Security Implications
No significant security implications are expected. The `.helmlintignore` file is processed locally by `helm lint` without modifying chart content or involving network activity.
## How to Teach This
Documentation for the `.helmlintignore` file and the `--lint-ignore-file` flag will be included in the official Helm docs under the `helm lint` section. Examples and common patterns will be provided to aid new users.
## Reference Implementation
No reference implementation yet. Post-acceptance, an issue will be created in the Helm community GitHub repository to add the HIP
danilo-patrucco marked this conversation as resolved.
Show resolved Hide resolved
## Open Issues
- Finalizing an effective and user-friendly pattern syntax for the `.helmlintignore` file.
- Potential inclusion of in-file annotations for temporarily excluding specific chart sections from linting.
## Rejected Ideas
None at this time.
## References
- This proposal is in line with the practices outlined in [HIP-0001](https://github.com/helm/community/blob/master/hips/hip-0001.md).