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

Add golangci-lint to github workflow #7556

Closed
wants to merge 2 commits into from

Conversation

omerap12
Copy link
Member

@omerap12 omerap12 commented Dec 3, 2024

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Adds golangci-lint to verify code quality along with golint (ref: #7542 (comment))

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 3, 2024
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 3, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: omerap12
Once this PR has been reviewed and has the lgtm label, please assign gjtempleton for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 3, 2024
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 3, 2024
@omerap12 omerap12 changed the title Add golangci-lint to verify-golint.sh script Add golangci-lint to github workflow Dec 3, 2024
@omerap12
Copy link
Member Author

omerap12 commented Dec 3, 2024

I've added this action: golangci-lint-action v6. However, I’m not happy with the duplicated code—perhaps someone can suggest a better approach.

One possibility is to create a separate GitHub workflow file to run the linter in parallel using a matrix configuration.
example:

lint:
  runs-on: ubuntu-latest
  strategy:
    matrix:
      dir: [vertical-pod-autoscaler, cluster-autoscaler, addon-resizer, balancer, multidimensional-pod-autoscaler]
  steps:
    - name: Set up Go
      uses: actions/setup-go@v2
      with:
        go-version: '1.22.2'

    - uses: actions/checkout@v2
      with:
        path: ${{ env.GOPATH }}/src/k8s.io/autoscaler

    - name: golangci-lint
      uses: golangci/golangci-lint-action@v6
      with:
        working-directory: ${{ env.GOPATH }}/src/k8s.io/autoscaler/${{ matrix.dir }}
        version: v1.60

Alternatively, we could skip GitHub Actions entirely and integrate this manually into the existing script: verify-golint.sh.

Any thoughts or suggestions?
cc @adrianmoisey @voelzmo

@raywainman
Copy link
Contributor

FWIW I think the way you have it here is OK.

Moving these up into GitHub Actions seems like the right path forward to me, the UX is also slightly better that way with a nice breakdown in each PR for each action.

So overall direction here LGTM.

@adrianmoisey
Copy link
Member

I'm also fine with the duplication, it isn't that big of a deal

@adrianmoisey
Copy link
Member

Could you enable checks? I want to see what the annotations look like: https://github.com/golangci/golangci-lint-action?tab=readme-ov-file#annotations

Comment on lines 196 to 201
func parseVpaObservedContainers(pod *apiv1.Pod) (bool, sets.String) {
observedContainers, hasObservedContainers := pod.GetAnnotations()[annotations.VpaObservedContainersLabel]
vpaContainerSet := sets.New[string]()
vpaContainerSet := sets.NewString()
if hasObservedContainers {
if containers, err := annotations.ParseVpaObservedContainersValue(observedContainers); err != nil {
klog.ErrorS(err, "VPA annotation failed to parse", "pod", klog.KObj(pod), "annotation", observedContainers)
Copy link
Member Author

Choose a reason for hiding this comment

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

I have changed this just for testing purposes . Ill will revert this before merge

@omerap12
Copy link
Member Author

omerap12 commented Dec 4, 2024

Could you enable checks? I want to see what the annotations look like: https://github.com/golangci/golangci-lint-action?tab=readme-ov-file#annotations

done

@adrianmoisey
Copy link
Member

I tested out annotations in https://github.com/kubernetes/autoscaler/pull/7558/files, I like them

Signed-off-by: Omer Aplatony <[email protected]>
@adrianmoisey
Copy link
Member

Hmmm... balancer is failing.
May be this needs to be rolled out project at a time?

@omerap12
Copy link
Member Author

omerap12 commented Dec 4, 2024

@adrianmoisey , added.
I will raise another PR to fix all those golang-lint fixes.

@omerap12
Copy link
Member Author

omerap12 commented Dec 4, 2024

Hmmm... balancer is failing. May be this needs to be rolled out project at a time?

Hmmm... balancer is failing. May be this needs to be rolled out project at a time?

GitHub Actions PRs can be hard to merge, so I tried to do everything in one PR. I don’t mind splitting it into a couple of PRs if that works better. @raywainman, what do you think?

@adrianmoisey
Copy link
Member

I don’t mind splitting it into a couple of PRs if that works better.

For approval sake, it may make sense to make a PR per project, and once they are merged, add the GitHub Action

@omerap12
Copy link
Member Author

omerap12 commented Dec 4, 2024

So I'm closing this PR for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/vertical-pod-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants