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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions .github/workflows/lint-sample.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ jobs:
- name: Prepare ${{ matrix.folder }}
working-directory: ${{ matrix.folder }}
run: go mod tidy
- name: Check linter configuration
working-directory: ${{ matrix.folder }}
run: make lint-config
- name: Run linter
uses: golangci/golangci-lint-action@v6
with:
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ jobs:
uses: actions/setup-go@v5
with:
go-version-file: go.mod
- name: Check linter configuration
run: make lint-config
- name: Run linter
uses: golangci/golangci-lint-action@v6
with:
Expand Down
8 changes: 6 additions & 2 deletions .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ 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
disable:
- fieldalignment
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@camilamacedo86 as configuration was wrong, it was not enforcing fieldalignment rule

I disabled it because there are several files affected

I suggest we merge this one, and fix the linter errors in follow up

- nilness
- shadow
revive:
rules:
# The following rules are recommended https://github.com/mgechev/revive#recommended-configuration
Expand All @@ -28,7 +32,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
4 changes: 4 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@ 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
$(GOLANGCI_LINT) run
Expand Down
4 changes: 4 additions & 0 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,6 +82,10 @@ 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
$(GOLANGCI_LINT) run
Expand Down
4 changes: 4 additions & 0 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,6 +78,10 @@ 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
$(GOLANGCI_LINT) run
Expand Down
4 changes: 4 additions & 0 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,6 +82,10 @@ 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
$(GOLANGCI_LINT) run
Expand Down
4 changes: 4 additions & 0 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 @@ -157,6 +157,10 @@ 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
$(GOLANGCI_LINT) run
Expand Down
4 changes: 4 additions & 0 deletions testdata/project-v4-multigroup/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ 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
$(GOLANGCI_LINT) run
Expand Down
4 changes: 4 additions & 0 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,6 +78,10 @@ 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
$(GOLANGCI_LINT) run
Expand Down
4 changes: 4 additions & 0 deletions testdata/project-v4/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ 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
$(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

Expand Down
Loading