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

lint: Add --kube-version flag to set capabilities and deprecation rules #10677

Merged
merged 2 commits into from Jan 9, 2024

Conversation

antoinedeschenes
Copy link
Contributor

@antoinedeschenes antoinedeschenes commented Feb 15, 2022

Signed-off-by: Antoine Deschênes antoine@antoinedeschenes.com

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

Adds a --kube-version flag to the helm lint command, allowing to specify the Kubernetes version to use for the deprecation check warnings.

There's a similar flag in the helm template command.

Specific example of where this is useful:

  • helm 3.8.0 releases are currently targetting Kubernetes 1.23.
  • We're currently running Kubernetes 1.22 clusters.
  • helm lint --strict is currently erroring out on HorizontalPodAutoscaler autoscaling/v2beta2 having to be replaced by autoscaling/v2.
  • autoscaling/v2 doesn't exist yet on Kubernetes 1.22.

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/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 15, 2022
@antoinedeschenes antoinedeschenes force-pushed the lint-kube-version branch 2 times, most recently from caee60c to 0334156 Compare February 15, 2022 16:36
pkg/lint/lint.go Outdated Show resolved Hide resolved
@yxxhero
Copy link
Member

yxxhero commented Mar 31, 2022

ping @antoinedeschenes

@antoinedeschenes antoinedeschenes marked this pull request as ready for review May 16, 2022 21:07
@antoinedeschenes
Copy link
Contributor Author

@yxxhero Just reversed All and Templates signatures to the original, added new functions in parallel.

pkg/lint/rules/template.go Outdated Show resolved Hide resolved
pkg/lint/lint.go Outdated Show resolved Hide resolved
@antoinedeschenes
Copy link
Contributor Author

pign @yxxhero

Copy link
Contributor

@joejulian joejulian left a comment

Choose a reason for hiding this comment

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

Nice. This is what I originally had hoped to do when I added the upstream deprecation library. Thanks!

@joejulian joejulian added this to the 3.11.0 milestone Oct 14, 2022
@antoinedeschenes
Copy link
Contributor Author

@joejulian Awesome, do you need a rebase?

@joejulian
Copy link
Contributor

Not according to GitHub

@pete-woods
Copy link

pete-woods commented Nov 23, 2022

@yxxhero I don't like to nag, but this PR adds something super useful for us, and has been waiting for some time

@joejulian
Copy link
Contributor

@pete-woods @yxxhero and I are just triage maintainers. You need a core maintainer to review.

@yxxhero
Copy link
Member

yxxhero commented Nov 24, 2022

@joejulian me too.

@pete-woods
Copy link

Ah. Didn't appreciate that - apologies

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.

Thanks for this PR. I've long wanted this feature and am excited for it to land.

I found one minor issue that shows up for SDK consumers. Should be a quick fix if you could please make the change. Otherwise it looks good to me.

pkg/lint/rules/deprecations.go Outdated Show resolved Hide resolved
@mattfarina
Copy link
Collaborator

@antoinedeschenes can you make the one small change or should someone else make it?

@mattfarina mattfarina modified the milestones: 3.11.0, 3.12.0 Jan 18, 2023
@joejulian joejulian modified the milestones: 3.12.0, 3.13.0 May 5, 2023
@antoinedeschenes
Copy link
Contributor Author

Anything missing?

@zifter
Copy link

zifter commented Aug 28, 2023

Any updates?

@joejulian
Copy link
Contributor

Tested this and it works for me. I wish we could make it an error if it's past the unavailable deadline, but that would be a breaking change, I guess, and outside the scope of this PR.

@joejulian joejulian added the Has One Approval This PR has one approval. It still needs a second approval to be merged. label Aug 28, 2023
@mattfarina mattfarina modified the milestones: 3.13.0, 3.14.0 Sep 25, 2023
@antoinedeschenes
Copy link
Contributor Author

Any timeframe in mind?

@mattfarina
Copy link
Collaborator

Note, I fixed a merge conflict caused by #12688 where strict became _. An unused argument. This was to pass linting.

@mattfarina
Copy link
Collaborator

Since my commit should not have made major changes to cause the failed test run, I checked out the commit before it and ran the test suite there. I get the same error of...

--- FAIL: TestLintCmdWithKubeVersionFlag (0.01s)
    --- FAIL: TestLintCmdWithKubeVersionFlag/lint_chart_with_deprecated_api_version_without_kube_version (0.00s)
        helm_test.go:61: running cmd (attempt 1): lint testdata/testcharts/chart-with-deprecated-api
        helm_test.go:70: does not match golden file testdata/output/lint-chart-with-deprecated-api.txt

            WANT:
            '==> Linting testdata/testcharts/chart-with-deprecated-api
            [INFO] Chart.yaml: icon is recommended
            [WARNING] templates/horizontalpodautoscaler.yaml: autoscaling/v2beta1 HorizontalPodAutoscaler is deprecated in v1.22+, unavailable in v1.25+; use autoscaling/v2 HorizontalPodAutoscaler

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

            GOT:
            '==> Linting testdata/testcharts/chart-with-deprecated-api
            [INFO] Chart.yaml: icon is recommended

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

This is for commit 46e3886.

This is because the internal k8s version is set to 1.20 which is what's used for the testing. So the lint command without setting the Kubernetes version uses this which would not produce the error.

I'm not sure how this test ever passed.

The easy fix is to update k8sVersionMinor.

Signed-off-by: Antoine Deschênes <antoine@antoinedeschenes.com>
Signed-off-by: Joe Julian <me@joejulian.name>
@joejulian
Copy link
Contributor

Since my commit should not have made major changes to cause the failed test run, I checked out the commit before it and ran the test suite there. I get the same error of...

--- FAIL: TestLintCmdWithKubeVersionFlag (0.01s)
    --- FAIL: TestLintCmdWithKubeVersionFlag/lint_chart_with_deprecated_api_version_without_kube_version (0.00s)
        helm_test.go:61: running cmd (attempt 1): lint testdata/testcharts/chart-with-deprecated-api
        helm_test.go:70: does not match golden file testdata/output/lint-chart-with-deprecated-api.txt

            WANT:
            '==> Linting testdata/testcharts/chart-with-deprecated-api
            [INFO] Chart.yaml: icon is recommended
            [WARNING] templates/horizontalpodautoscaler.yaml: autoscaling/v2beta1 HorizontalPodAutoscaler is deprecated in v1.22+, unavailable in v1.25+; use autoscaling/v2 HorizontalPodAutoscaler

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

            GOT:
            '==> Linting testdata/testcharts/chart-with-deprecated-api
            [INFO] Chart.yaml: icon is recommended

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

This is for commit 46e3886.

This is because the internal k8s version is set to 1.20 which is what's used for the testing. So the lint command without setting the Kubernetes version uses this which would not produce the error.

I'm not sure how this test ever passed.

The easy fix is to update k8sVersionMinor.

This should have made it work: 869c1d2#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R107

But it broke a bunch of other golden images that have labels with the k8s version data.

Rather than have to update the golden files with every client-go release, I reverted the addition of ldflags on the test and switched the expected golden image to the old k8s one.

@joejulian
Copy link
Contributor

sorry for my force-pushes. I neglected to sign my commit and am trying to help get this across the finish line for this release.

@mattfarina mattfarina merged commit 1ccde5a into helm:main Jan 9, 2024
6 checks passed
@antoinedeschenes antoinedeschenes deleted the lint-kube-version branch January 9, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Has One Approval This PR has one approval. It still needs a second approval to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

helm lint should allow specifying --kube-version flag
7 participants