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.

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

do not check YAML if nothing was parsed #8913

Merged
merged 3 commits into from
Nov 6, 2020
Merged

Conversation

cvila84
Copy link

@cvila84 cvila84 commented Oct 18, 2020

What this PR does / why we need it:
closes #8621

Special notes for your reviewer:
added a test to avoid lint rules to be applied on parsed empty object

If applicable:

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

@helm-bot helm-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 18, 2020
@cvila84
Copy link
Author

cvila84 commented Oct 18, 2020

CircleCI build is failing because I don't have Azure environment for deployment (missing AZURE_STORAGE_CONNECTION_STRING)

@mattfarina
Copy link
Collaborator

@cvila84 This appears to be because you setup your own CircleCI for your fork of Helm. You should let the setup for the repo at github.com/helm/helm handle it.

When the CI run is for a pull request the deploy step is skipped. But, you ran it for a branch as setup in your own environment.

Does this make sense?

@cvila84
Copy link
Author

cvila84 commented Oct 20, 2020

@mattfarina This is crystal clear. I just don't remember to have setup myself a project in my circle CI dashboard. I suspect it was done automatically when I pushed for the first time the new 'fork-lint-bug' branch in my fork as it contained the .circleci/config.xml... Anyway, I removed it, as it does not make sense as you explained

@bacongobbler
Copy link
Member

I noticed this branch is based on the release-3.3 branch instead of master. Please re-target this PR against the master branch. That may also be the cause why the CI pipeline failed. Thanks.

@bacongobbler bacongobbler changed the base branch from release-3.3 to master October 21, 2020 15:14
@bacongobbler
Copy link
Member

Okay. It looks like I was able to do that myself. Now it just looks like you need to rebase a0df4ba on top of master for CI to run correctly.

@cvila84
Copy link
Author

cvila84 commented Oct 21, 2020

@bacongobbler seems much better now !

@mattfarina mattfarina added the bug Categorizes issue or PR as related to a bug. label Oct 27, 2020
@mattfarina mattfarina added this to the 3.4.1 milestone Oct 27, 2020
@mattfarina
Copy link
Collaborator

@cvila84 could you please add a test for this situation. We want to make sure something in the future doesn't accidentally undo this causing a regression.

@helm-bot helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 27, 2020
@cvila84
Copy link
Author

cvila84 commented Oct 27, 2020

@mattfarina I added 1 UT. I also tested that this UT fails against both 3.3.4 and 3.4.0 release.

Copy link
Collaborator

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

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

lgtm

@hickeyma
Copy link
Contributor

hickeyma commented Nov 5, 2020

Is this PR to handle a yaml file:

  • which contains a comment only
  • which starts with #@formatter:off

For testing:

I created a scaffold chart. I added a file with comment only and updated the deployment.yaml file with the formatter as follows:

$ cat issue-8909/templates/empty.yaml
# This is an empty template

$ cat issue-8909/templates/deployment.yaml
#@formatter:off
apiVersion: apps/v1
kind: Deployment
metadata:
[...]

I tested as follows first without applying the patch:

$ helm3 lint issue-8909
==> Linting issue-8909
[INFO] Chart.yaml: icon is recommended
[ERROR] templates/empty.yaml: object name must be between 0 and 253 characters: ""

Error: 1 chart(s) linted, 1 chart(s) failed

I then tested with the PR as follows:

$ helm3 lint issue-8909
==> Linting issue-8909
[INFO] Chart.yaml: icon is recommended

1 chart(s) linted, 0 chart(s) failed

Is this the fix as expected? Does this PR fix #8621 and if so then is PR #8862 actually not fixing #8621?

@cvila84
Copy link
Author

cvila84 commented Nov 5, 2020

This PR is fixing for sure #8621. It is very likely PR #8862 also fixes this issue, but the updates of this other PR implies other changes (on YAML parser) that needs to be reviewed/accepted before whereas this one is just a simple fix

@hickeyma
Copy link
Contributor

hickeyma commented Nov 5, 2020

@cvila84 I agree that this looks like it does. I am however unsure about the other PR at this moment. I have raised to discuss it today at the dev weekly.

@hickeyma
Copy link
Contributor

hickeyma commented Nov 6, 2020

Both #8913 and #8862 fix #8621. PR #8862 deals also with multi-doc yaml.

As discussed with @mattfarina at dev weekly meeting yesterday, it as agreed to merge both PRs as there are additional unit tests in this PR, and therefore adds value.

Copy link
Contributor

@hickeyma hickeyma left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix @cvila84

@hickeyma hickeyma merged commit 6ca989e into helm:master Nov 6, 2020
@hickeyma hickeyma added the needs-pick Indicates that a PR needs to be cherry-picked into the next release candidate. label Nov 6, 2020
@cvila84
Copy link
Author

cvila84 commented Nov 6, 2020

you're welcome :)

@mattfarina mattfarina added the picked Indicates that a PR has been cherry-picked into the next release candidate. label Nov 11, 2020
mattfarina pushed a commit that referenced this pull request Nov 11, 2020
Signed-off-by: Christophe VILA <[email protected]>
(cherry picked from commit 8a4c0bc)
clrpackages pushed a commit to clearlinux-pkgs/helm that referenced this pull request Nov 24, 2020
Abhilash Gnan (6):
      add time-format flag to list command
      fix ls command example
      refactor time formatting
      rename to time format flag
      fix example time format
      remove redudant time func

Adam Reese (2):
      fix(cmd/helm): add build tags for architecture
      ref(cmd): prevent klogs flags from polluting the help text

Andrew Melis (2):
      Make helm ls return only current releases if providing state filter
      Enhance readability by extracting bitwise operation

Bridget Kromhout (8):
      Clarify comments to match practice
      Fixing version and spelling errors
      Correct make target in Makefile
      Reinstating comment that is still accurate
      Linking to a more complete list of meeting details.
      Updating descriptions
      Clarifies action needed to list new stable repo
      List either incubator or stable.

Calle Pettersson (1):
      Report what cause finding chart to fail

Chris Wells (1):
      fix: Allow building in a path containing spaces

Christophe VILA (2):
      do not check YAML if nothing was parsed
      added test for helm/helm#8913 related to helm/helm#8621

Cristian Klein (3):
      fix(helm): Avoid corrupting storage via a lock
      fix(helm): Added test for concurrent upgrades
      fix(helm): Update test during pending install

Dmitry Chepurovskiy (7):
      Added selector option to list command
      Pass labels from secret/configmap to release object
      Added selector filtering
      Fix linting issues
      Added notice about supported backends
      Added testing for list action with selector
      Move selector filtering after latest release version filtering

Dong Gang (5):
      feature(show): support jsonpath output for `helm show value`
      fix the code style error
      polish the help text of flag
      fix(create): update the hook name of test-connection pod
      polish the error handler

Erik Sundell (1):
      helm create: make generated YAML indentation more consistent

Haoming Zhang (1):
      Close gzip reader when done.

Hidde Beydals (1):
      fix(kube): use logger instead of fmt.Printf

Holder (1):
      fix(sdk): Polish the downloader/manager package error return (#8491)

Jack Weldon (3):
      feat(helm): prompt for password with helm repo add
      address PR comment, adding whitespace for formatting
      fix windows build failure caused by #8431

Janario Oliveira (2):
      Reuse kube-client
      Adjusted import

Jinesi Yelizati (1):
      chore(deps): add dependabot.yml

Josh Dolitsky (1):
      Add GPG signature verification to install script (#7944)

Lehel Gyuro (2):
      [#7696] Avoid crash in chart loader on unexpected file sequence
      [#7696] Avoid crash in chart loader on unexpected file sequence

Li Zhijian (8):
      Use T.cleanup() to cleanup helm-action-test
      Use T.Cleanup() to cleanup temp dir helm-repotest
      mark NewTempServer as Deprecated
      Use RemoveAll to remove a non-empty directory
      Use T.cleanup() to cleanup cmdtest_temp file
      fix incorrect wildcard expand
      TestCheckPerms: utilize pipe to read stderr
      Makefile: check and use GOBIN environment variable first

Ling Samuel (1):
      feat: status command display description

Liu Ming (1):
      Remove duplicate variable definition

Ma Xinjian (3):
      darwin-386 and windows-386 are not supported now
      Correct checksum file links
      Add support to install helm

Maartje Eyskens (1):
      Bump Kubernetes to v0.18.8 + Bump jsonpatch

Manuel Rüger (1):
      pkg/*: Small linting fixes

Marc Khouzam (3):
      feat(comp): Disable file completion when not valid
      feat(comp): Add support for fish completion
      fix(comp): Disable file comp for output formats

Martin Hickey (4):
      Fix typo
      Revert PR 8562
      Fix the lint error message for valid names
      Update err message to use the regex pattern directly

Matt Butcher (18):
      Lint dependencies (#7970)
      fix test that modifies the wrong cache data
      fix name length check on lint (#8543)
      Fix/8467 linter failing (#8496)
      Fixed a variable name collision caused by two PR merges (#8681)
      fix: check mode bits on kubeconfig file
      validate the name passed in during helm create
      refactor the release name validation to be consistent across Helm
      fixed bug that caused helm create to not overwrite modified files
      handle case where dependency name collisions break dependency resolution
      replace --no-update with --force-update and invert default. BREAKING.
      improve the HTTP detection for tar archives
      Merge pull request from GHSA-9vp5-m38w-j776
      Merge pull request from GHSA-jm56-5h66-w453
      Merge pull request from GHSA-m54r-vrmv-hw33
      improved user-facing error messages to explain the underlying problem (#8731)
      warn and block old repo URLs (#8903)
      this rewrites a whole bunch of old repo URLs to the new repo URL (#8902)

Matt Farina (16):
      Locking file URIs to a version in lockfile
      Make unmanaged repositories version resolvable
      Adding helm v4 todo
      Restoring Build behavior
      Fix issue with install and upgrade running all hooks
      Fixing linting of templates on Windows
      Fixing failing CI for windows
      Fixing issue with idempotent repo add
      Adding size labels pointer
      Fixing import package issue
      Adding annotation to index.yaml file
      Adding support for k8s 1.19
      bump version to v3.4.0
      Fixes Error: could not find protocol handler for
      Updating to k8s 1.19.3 based packages
      Added tests for PR 8948

Matthew Fisher (10):
      bump version to v3.3
      introduce stale issue bot
      add v4 to list of exempt labels
      use URL escape codes
      go fmt
      switched to stricter YAML parsing on plugin metadata files
      fix: allow serverInfo field on index files
      size/S and larger requiring 2 LGTMs
      use warning function
      add authentication to CircleCI jobs

Michelle Noorali (1):
      chore(OWNERS): move michelleN to emeritus

Mikuláš Dítě (1):
      feat(install): add requested version to error

Morten Linderud (2):
      release/mock: Ensure conversion from int to string yields a string
      Makefile: Fix LDFLAGS overriding

Nandor Kracser (1):
      lint: lint all documents in a multi-doc yaml file

Paul Brousseau (1):
      Fixing typo in engine comments

Rajat Jindal (1):
      fix watch error due to elb/proxy timeout

Scott Rigby (1):
      Update docs links in release notes script

Sebastian Sdorra (1):
      support passing signing passphrase from file or stdin (#8394)

She Jiayu (1):
      Avoid hardcoded container port in default notes

Simon Alling (1):
      Simplify chart installable check

Simon Shine (1):
      Alter whitespace in "Update Complete" output

Tariq Ibrahim (1):
      optimise if condition in service ready logic

Tero (1):
      Fix Quick Start Guide Link in README.md

Thomas O'Donnell (1):
      Update Common Lables template in starter chart

Thulio Ferraz Assis (2):
      fix: with .Values.podAnnotations indent template
      fix: if not .Values.autoscaling.enabled indent

Yuchen Cheng (1):
      add checkFileCompletion for env HELM_BIN

Zhengyi Lai (2):
      Bugfix: panic when chart contains requirements.lock
      Add test case for LoadFiles

Zhou Hao (1):
      cleanup tempfiles for load_test

bellkeyang (1):
      bufix: fix validateNumColons docs

dependabot[bot] (4):
      Bump github.com/lib/pq from 1.7.0 to 1.8.0
      Bump github.com/gofrs/flock from 0.7.1 to 0.8.0
      Bump github.com/sirupsen/logrus from 1.6.0 to 1.7.0
      Bump github.com/DATA-DOG/go-sqlmock from 1.4.1 to 1.5.0

knrt10 (1):
      Fix spelling in completion.go

leigh capili (2):
      Document all env vars for CLI help
      Support impersonation via flags similar to kubectl --as="user"

lemonli (1):
      Update go version to 1.14 in go.mod

rudeigerc (1):
      feat(env): add support of accepting a specific variable for helm env

wawa0210 (4):
      Environment variable for setting the max history for an environment
      Rollback command support for max history
      Fix that the invalid version number of the helm package command will escape
      helm search supports semver pre version numbers starting with 0

yxxhero (5):
      fix insecure-skip-tls-verify flag does't work on helm install, Keep
      add unit tests for FindChartInAuthAndTLSRepoURL.
      add helm v4 todo comments for FindChartInAuthAndTLSRepoURL.
      Add --skip-refresh option in helm dep build
      add unittes for 'helm dep build' with --skip-refresh flag.

zouyu (1):
      Fix wrong function's name in comment

zwwhdls (5):
      fix #6116
      add test case
      fix another extreme case
      add test case
      fix conflict
clrpackages pushed a commit to clearlinux-pkgs/helm that referenced this pull request Feb 9, 2021
Aayush Joglekar (1):
      Add remaining tests in TestDependentChartAliases

Adam Reese (2):
      fix(*): Validate metadata semver and printable characters
      chore(go.mod): bump Masterminds/{spring,goutils} and deislabs/oras

Bridget Kromhout (3):
      Updating descriptions
      Clarifies action needed to list new stable repo
      List either incubator or stable.

Chris Aniszczyk (1):
      Add CodeQL Security Scanning

Chris Wells (1):
      feat: Allow helm test to run a subset of tests

Christophe VILA (2):
      do not check YAML if nothing was parsed
      added test for helm/helm#8913 related to helm/helm#8621

Daniel Lipovetsky (1):
      Add explanatory comments to action.List and action.History

Dinu Mathai (1):
      Fixed bug - The flags --cert-file/--key-file where ignored when --insecure-skip-tls-verify flag is set (#9070)

Janario Oliveira (2):
      Reuse kube-client
      Adjusted import

Joe Julian (1):
      Reduce linting severity for users of out-of-date kubernetes (#8608)

Jon Huhn (1):
      Fix typo

Josh Dolitsky (1):
      Upgrade to oras v0.9.0 (#9269)

Lehel Gyuro (2):
      [#7696] Avoid crash in chart loader on unexpected file sequence
      [#7696] Avoid crash in chart loader on unexpected file sequence

Lüchinger Dominic (1):
      Adds the option kube-cafile and env variable HELM_KUBECAFILE for a overwrite of the certificate authority file

Ma Xinjian (2):
      Add support to judge whether desired version is available or not
      Cleanup tempfiles introduced by unit tests under pkg/

Marc Khouzam (5):
      feat(test): Adapt completion tests to Cobra 1.1
      fix(helm): flag descriptions start with lowercase
      chore(comp): Remove unnecessary completion code
      feat(helm): Allow generating markdown docs headers
      fix(pkg/chartutil): Remove warning for nils

Martin Hickey (2):
      Fix the lint error message for valid names
      Update err message to use the regex pattern directly

Matt Farina (13):
      bump version to v3.4.0
      Fixes Error: could not find protocol handler for
      Updating to k8s 1.19.3 based packages
      Added tests for PR 8948
      Updating to Kubernetes 1.19.4 package versions
      Builds with go 1.15
      Updating to Kuberentes 1.20 packages
      Updating to sprig 3.2.0
      Bumping kubernetes to 1.20.1
      Adding apiserver to mod/sum
      bump version to
      Fix dep build with OCI based charts
      Adding missing replace directive for oras

Matthew Fisher (4):
      fix(test): display error message
      increase number of operations per run to 100
      bump actions/stale to v3.0.14
      Revert "Add support to judge whether desired version is available or not"

Nandor Kracser (1):
      lint: lint all documents in a multi-doc yaml file

Peter Engelbert (4):
      Implement `helm pull` for OCI registries
      Clean up imports and add doc comments
      Remove OCI boolean from  struct
      Address error on deletion of old dependencies

Salim Salaues (1):
      fix: ingress path issue

Scaat Feng (1):
      [COMMENT]fix comment

Scott Rigby (1):
      Replace Helm Hub with Artifact Hub (#8626)

Torsten Walter (3):
      prepare testdata
      fix(helm): allow skipping manifests in tests directories
      Skip tests when running helm template

Zhengyi Lai (2):
      Bugfix: panic when chart contains requirements.lock
      Add test case for LoadFiles

dependabot[bot] (4):
      Bump github.com/spf13/cobra from 1.0.0 to 1.1.1
      Bump github.com/lib/pq from 1.8.0 to 1.9.0 (#9107)
      Bump github.com/Masterminds/squirrel from 1.4.0 to 1.5.0 (#9108)
      Bump github.com/Masterminds/semver/v3 from 3.1.0 to 3.1.1 (#9109)

knrt10 (1):
      completion: move to native zshCompletion

rimas (2):
      Fixes #9083
      Fix test

wawa0210 (2):
      helm search supports semver pre version numbers starting with 0
      Fix that the invalid version number of the helm package command will escape

yxxhero (2):
      Add --skip-refresh option in helm dep build
      add unittes for 'helm dep build' with --skip-refresh flag.

zh168654 (4):
      helm upgrade with --wait support jobs in manifest to be completed
      add test cases
      add wait-for-jobs flag
      add waitwithjobs instead of changing wait api

zhangye15 (2):
      fix test-style error
      fix style conformance
zak905 pushed a commit to zak905/helm that referenced this pull request Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Categorizes issue or PR as related to a bug. needs-pick Indicates that a PR needs to be cherry-picked into the next release candidate. picked Indicates that a PR has been cherry-picked into the next release candidate. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linting fails in 3.3.0 when a template file results in only a comment.
5 participants