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

✨ (go/v4): Add new makefile target to check and validate the linter config #4425

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+48 −12
Open

✨ (go/v4): Add new makefile target to check and validate the linter config #4425

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d768290
fix: Check linter configuration
mateusoliveira43 Dec 13, 2024
15fb1c1
fixup! fix: Check linter configuration
mateusoliveira43 Dec 13, 2024
3592383
fixup! fix: Check linter configuration
mateusoliveira43 Dec 13, 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
20 changes: 12 additions & 8 deletions .github/workflows/lint-sample.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,13 @@ on:
jobs:
lint-samples:
runs-on: ubuntu-latest
strategy:
matrix:
folder: [
"testdata/project-v4",
"testdata/project-v4-with-plugins",
"testdata/project-v4-multigroup"
]
if: (github.event_name == 'push' || github.event.pull_request.head.repo.full_name != github.repository)
steps:
- name: Clone the code
Expand All @@ -22,12 +29,9 @@ jobs:
- name: Run linter
uses: golangci/golangci-lint-action@v6
with:
version: v1.59
working-directory: testdata/project-v4
args: --config .golangci.yml ./...
- name: Run linter
uses: golangci/golangci-lint-action@v6
with:
version: v1.59
working-directory: testdata/project-v4-with-plugins
version: v1.61.0
working-directory: ${{ matrix.folder }}
args: --config .golangci.yml ./...
mateusoliveira43 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

HI @mateusoliveira43

I gave an wrong direction here sorry that is 100% my fault
We will need

      - name: Run linter
        uses: golangci/golangci-lint-action@v6
        with:
          version: v1.59
          working-directory: testdata/project-v4-with-plugins
          args: --config .golangci.yml ./...

Otheriwse, if we broke the lint in the project that we scaffold we will not see GitHub comments

- name: Check linter configuration
working-directory: ${{ matrix.folder }}
run: make lint-config
4 changes: 3 additions & 1 deletion .github/workflows/lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ jobs:
- name: Run linter
uses: golangci/golangci-lint-action@v6
with:
version: v1.61
version: v1.61.0
- name: Check linter configuration
run: make lint-config

yamllint:
runs-on: ubuntu-latest
Expand Down
6 changes: 3 additions & 3 deletions .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ issues:

linters-settings:
govet:
enable=fieldalignment: true
Copy link
Member

@camilamacedo86 camilamacedo86 Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@mateusoliveira43 this change seems unrelated to the scope right?
Seems that if we change it, then we will start to fail where we did not before
Is that falling under the new target to validate the configuration?
Why do we need to do this change?

If we need to make this change, we need to fix the issues as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, looking CI errors, I think the only complain is indeed fieldalignment errors (I do not know yet why lint-sample failed)

should I disable it?

Copy link
Member

Choose a reason for hiding this comment

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

We should not disabled the lint-simple
The lint sample has the purpose of lint the test samples and ensuring that the code that we generate with the tool passes the checks.

Why we need to change enable=fieldalignment: true can we please revert that it does not seems part of the scope then, if you wish change it we can do in a follow up where the issues are fixed and we explain why one option should be used instead of another

Copy link
Member

Choose a reason for hiding this comment

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

Moreover, we need to have a new target for the Makefile that we use for Kubebuilder as well.
It can be here, and we do all by once, for our samples and tooling or can be in a follow up PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh no, I mean disable fieldalignment rule

If you check lint job logs, all errors were related to this rule

I made the change because golangci-lint config verify complained about this configuration

but we can change this part of project Makefile in a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

undid changes to configuration, just added new command to project Makefile, without calling it in CI

Copy link
Member

Choose a reason for hiding this comment

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

I made the change because golangci-lint config verify complained about this configuration

Should it not fail in the CI if the lint raises errors and we revert the change?

enable-all: true
revive:
rules:
# The following rules are recommended https://github.com/mgechev/revive#recommended-configuration
Expand All @@ -28,7 +28,7 @@ linters-settings:
- name: dot-imports
arguments:
# dot import should be ONLY allowed for ginkgo testing packages
allowedPackages:
- allowedPackages:
- "github.com/onsi/ginkgo/v2"
- "github.com/onsi/gomega"
- name: error-return
Expand Down Expand Up @@ -66,9 +66,9 @@ linters-settings:
linters:
disable-all: true
enable:
- copyloopvar
- dupl
- errcheck
- copyloopvar
- ginkgolinter
- goconst
- gocyclo
Expand Down
8 changes: 6 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,16 @@ generate-charts: build ## Re-generate the helm chart testdata only
check-docs: ## Run the script to ensure that the docs are updated
./hack/docs/check.sh

.PHONY: lint-config
lint-config: golangci-lint ## Verify golangci-lint linter configuration
$(GOLANGCI_LINT) config verify

.PHONY: lint
lint: golangci-lint yamllint ## Run golangci-lint linter & yamllint
lint: lint-config yamllint ## Run golangci-lint linter & yamllint
$(GOLANGCI_LINT) run

.PHONY: lint-fix
lint-fix: golangci-lint ## Run golangci-lint linter and perform fixes
lint-fix: lint-config ## Run golangci-lint linter and perform fixes
$(GOLANGCI_LINT) run --fix

.PHONY: yamllint
Expand Down
8 changes: 6 additions & 2 deletions docs/book/src/cronjob-tutorial/testdata/project/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,16 @@ test-e2e: manifests generate fmt vet ## Run the e2e tests. Expected an isolated
}
go test ./test/e2e/ -v -ginkgo.v

.PHONY: lint-config
lint-config: golangci-lint ## Verify golangci-lint linter configuration
$(GOLANGCI_LINT) config verify

Copy link
Member

Choose a reason for hiding this comment

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

We change things for end users, so as it is an addition, we should use ✨ since it should be highlighted in the release notes.

.PHONY: lint
lint: golangci-lint ## Run golangci-lint linter
lint: lint-config ## Run golangci-lint linter
$(GOLANGCI_LINT) run

.PHONY: lint-fix
lint-fix: golangci-lint ## Run golangci-lint linter and perform fixes
lint-fix: lint-config ## Run golangci-lint linter and perform fixes
$(GOLANGCI_LINT) run --fix

##@ Build
Expand Down
8 changes: 6 additions & 2 deletions docs/book/src/getting-started/testdata/project/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,16 @@ test-e2e: manifests generate fmt vet ## Run the e2e tests. Expected an isolated
}
go test ./test/e2e/ -v -ginkgo.v

.PHONY: lint-config
lint-config: golangci-lint ## Verify golangci-lint linter configuration
$(GOLANGCI_LINT) config verify

.PHONY: lint
lint: golangci-lint ## Run golangci-lint linter
lint: lint-config ## Run golangci-lint linter
mateusoliveira43 marked this conversation as resolved.
Show resolved Hide resolved
$(GOLANGCI_LINT) run

.PHONY: lint-fix
lint-fix: golangci-lint ## Run golangci-lint linter and perform fixes
lint-fix: lint-config ## Run golangci-lint linter and perform fixes
mateusoliveira43 marked this conversation as resolved.
Show resolved Hide resolved
$(GOLANGCI_LINT) run --fix

##@ Build
Expand Down
8 changes: 6 additions & 2 deletions docs/book/src/multiversion-tutorial/testdata/project/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,16 @@ test-e2e: manifests generate fmt vet ## Run the e2e tests. Expected an isolated
}
go test ./test/e2e/ -v -ginkgo.v

.PHONY: lint-config
lint-config: golangci-lint ## Verify golangci-lint linter configuration
$(GOLANGCI_LINT) config verify

.PHONY: lint
lint: golangci-lint ## Run golangci-lint linter
lint: lint-config ## Run golangci-lint linter
$(GOLANGCI_LINT) run

.PHONY: lint-fix
lint-fix: golangci-lint ## Run golangci-lint linter and perform fixes
lint-fix: lint-config ## Run golangci-lint linter and perform fixes
$(GOLANGCI_LINT) run --fix

##@ Build
Expand Down
8 changes: 6 additions & 2 deletions pkg/plugins/golang/v4/scaffolds/internal/templates/makefile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,12 +155,16 @@ test-e2e: manifests generate fmt vet ## Run the e2e tests. Expected an isolated
}
go test ./test/e2e/ -v -ginkgo.v

.PHONY: lint-config
lint-config: golangci-lint ## Verify golangci-lint linter configuration
$(GOLANGCI_LINT) config verify

.PHONY: lint
lint: golangci-lint ## Run golangci-lint linter
lint: lint-config ## Run golangci-lint linter
$(GOLANGCI_LINT) run

.PHONY: lint-fix
lint-fix: golangci-lint ## Run golangci-lint linter and perform fixes
lint-fix: lint-config ## Run golangci-lint linter and perform fixes
$(GOLANGCI_LINT) run --fix

##@ Build
Expand Down
8 changes: 6 additions & 2 deletions testdata/project-v4-multigroup/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,16 @@ test-e2e: manifests generate fmt vet ## Run the e2e tests. Expected an isolated
}
go test ./test/e2e/ -v -ginkgo.v

.PHONY: lint-config
lint-config: golangci-lint ## Verify golangci-lint linter configuration
$(GOLANGCI_LINT) config verify

.PHONY: lint
lint: golangci-lint ## Run golangci-lint linter
lint: lint-config ## Run golangci-lint linter
$(GOLANGCI_LINT) run

.PHONY: lint-fix
lint-fix: golangci-lint ## Run golangci-lint linter and perform fixes
lint-fix: lint-config ## Run golangci-lint linter and perform fixes
$(GOLANGCI_LINT) run --fix

##@ Build
Expand Down
8 changes: 6 additions & 2 deletions testdata/project-v4-with-plugins/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,16 @@ test-e2e: manifests generate fmt vet ## Run the e2e tests. Expected an isolated
}
go test ./test/e2e/ -v -ginkgo.v

.PHONY: lint-config
lint-config: golangci-lint ## Verify golangci-lint linter configuration
$(GOLANGCI_LINT) config verify

.PHONY: lint
lint: golangci-lint ## Run golangci-lint linter
lint: lint-config ## Run golangci-lint linter
$(GOLANGCI_LINT) run

.PHONY: lint-fix
lint-fix: golangci-lint ## Run golangci-lint linter and perform fixes
lint-fix: lint-config ## Run golangci-lint linter and perform fixes
$(GOLANGCI_LINT) run --fix

##@ Build
Expand Down
8 changes: 6 additions & 2 deletions testdata/project-v4/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,16 @@ test-e2e: manifests generate fmt vet ## Run the e2e tests. Expected an isolated
}
go test ./test/e2e/ -v -ginkgo.v

.PHONY: lint-config
lint-config: golangci-lint ## Verify golangci-lint linter configuration
$(GOLANGCI_LINT) config verify

.PHONY: lint
lint: golangci-lint ## Run golangci-lint linter
lint: lint-config ## Run golangci-lint linter
$(GOLANGCI_LINT) run
Copy link
Member

@camilamacedo86 camilamacedo86 Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@mateusoliveira43

We have an error in the samples (main.go) that we need to ignore
//nolint:gocyclo could be scaffolded by default in the main,go

But if you have a better idea please feel free to try out


.PHONY: lint-fix
lint-fix: golangci-lint ## Run golangci-lint linter and perform fixes
lint-fix: lint-config ## Run golangci-lint linter and perform fixes
$(GOLANGCI_LINT) run --fix

##@ Build
Expand Down
Loading