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

Create lint and test CI for helm charts #357

Merged
merged 23 commits into from
Dec 17, 2024

Conversation

RafalKorepta
Copy link
Contributor

@RafalKorepta RafalKorepta commented Dec 12, 2024

Based on #354

9735fd3

Add go-tools, helm-docs and helm cli to nix definition. Copy most of task file from helm-charts.

1188a6e

Re-run helm-docs

85570bc

Add genpartial to go workspace.

885b0f5

Add type.Alias to genpartial as running genpartial shows that this type is unhandled and it panic.

5af3ee8

Add gotohelm to go workspace.

b960e79

Add genschema to go workspace.

b9b7c4a

Re-run charts generators (task charts:generate).

d6ac7f5

Re-run task charts:lint.

K8S-464

@RafalKorepta RafalKorepta force-pushed the rk/k8s-464/re-run-gotohelm-and-others branch 13 times, most recently from e584264 to 5f7628a Compare December 13, 2024 15:34
flake.nix Outdated Show resolved Hide resolved
.buildkite/helm.nix Outdated Show resolved Hide resolved
taskfiles/charts.yml Outdated Show resolved Hide resolved
taskfiles/charts.yml Outdated Show resolved Hide resolved
taskfiles/charts.yml Show resolved Hide resolved
@@ -226884,7 +226884,7 @@ spec:
- YJt9t
ip: UYcKtV
- ip: 4uIxYAS
- {}
- ip: ""
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels a bit suspect. Any idea where this change is coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea. I believe some of the changes in transpiler due to the fact we have multiple go modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

Figured this out in another PR of mine! It comes from updating the Kubernetes SDK. I think the IP field changed from *string to string.

@@ -39,3 +39,11 @@ dist/

# Ignore depedencies
charts/*/charts/*.tgz

# Ignore gotestsum junit reports
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we instead have this output to a single well known place? If we consolidated the test task (or perhaps we should have a ci:test task in this case) we could output this file to the root of the repo or else where instead of having to micromanage these ignores.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My initial plan was to use that, but then it's much easier to create buildkite matrix to test individual package and see where the failure is. Even re-try is much easier as you will not re-run the world (we don't have go test cache). It would be only that particular package.

.buildkite/pipeline.yml Outdated Show resolved Hide resolved
.buildkite/pipeline.yml Outdated Show resolved Hide resolved
.buildkite/pipeline.yml Outdated Show resolved Hide resolved
@RafalKorepta RafalKorepta force-pushed the rk/k8s-464/re-run-gotohelm-and-others branch 14 times, most recently from 3f41889 to 7fc859f Compare December 13, 2024 23:59
@RafalKorepta RafalKorepta force-pushed the rk/k8s-464/re-run-gotohelm-and-others branch 2 times, most recently from 575af04 to 05b2620 Compare December 16, 2024 21:21
@RafalKorepta RafalKorepta force-pushed the rk/k8s-464/re-run-gotohelm-and-others branch from 05b2620 to 7b49eb0 Compare December 17, 2024 10:23
Copy link
Contributor

@chrisseto chrisseto left a comment

Choose a reason for hiding this comment

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

Looks like go.work.sum got deleted somewhere along the way. Was that intentional?

@@ -7,6 +7,8 @@
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0

//go:build integration
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to add build tags to all these tests. Doing so messes with diagnostic tools and can hide build failures. Instead there's a single file in my PR with helpers that toggles a boolean but otherwise doesn't mess with the build.

@@ -226884,7 +226884,7 @@ spec:
- YJt9t
ip: UYcKtV
- ip: 4uIxYAS
- {}
- ip: ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Figured this out in another PR of mine! It comes from updating the Kubernetes SDK. I think the IP field changed from *string to string.

@@ -62,6 +64,9 @@
pkgs.kustomize
pkgs.kuttl
pkgs.openssl
pkgs.helm-docs
pkgs.helm-3-10-3
pkgs.go-junit-report
Copy link
Contributor

Choose a reason for hiding this comment

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

suuuuuuuuuuper nit: please sort this alphabetically :D

@@ -384,12 +387,15 @@ func FindAllNames(pkg *types.Package, root types.Type) []*types.Named {
push(current.Underlying())

switch current := current.(type) {
case *types.Basic, *types.Interface, *types.TypeParam:
case *types.Basic, *types.Interface, *types.TypeParam, *types.Alias:
Copy link
Contributor

Choose a reason for hiding this comment

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

types.Alias should be handled as the other types:

case *types.Alias:
    push(current.RHS())

// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0

//go:build integration
Copy link
Contributor

Choose a reason for hiding this comment

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

Same note about the build tag here but we can handle that after merging this PR.

@RafalKorepta RafalKorepta merged commit 26196e8 into main Dec 17, 2024
8 checks passed
@RafalKorepta RafalKorepta mentioned this pull request Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants