feat: add lint target to makefile #101

hessjcg · 2022-11-03T20:57:57Z

Introduce a new makefile target lint which will run generate and all lint checks
Introduce a lint check for go using golanci-lint
Introduce a lint check target for terraform using terraform fmt
Update the lint github action to use the new pr_check_lint target in the makefile

kurtisvg · 2022-11-07T02:31:52Z

.github/workflows/lint.yaml

+      - name: Verify no changes from lint tools. If you're reading this and the check has failed, run `make -f operator.mk pr_check_lint`.
+        run: make -f operator.mk pr_check_lint


Shouldn't this step happen after we remove the label?

cc @jackwotherspoon

Looks like I copied an older version of the workflow from cloud-sql-proxy. I'll move the remove label step up.

kurtisvg · 2022-11-07T02:32:48Z

.github/workflows/lint.yaml

@@ -36,8 +36,8 @@ jobs:
          repository: ${{ github.event.pull_request.head.repo.full_name }}
      - name: Set up build.env with phony secrets.
        run: cp build.sample.env build.env
-      - name: Verify no changes from lint tools. If you're reading this and the check has failed, run `make pre_commit`.
-        run: make github_lint
+      - name: Verify no changes from lint tools. If you're reading this and the check has failed, run `make -f operator.mk pr_check_lint`.


Shouldn't this just be generate? Won't generate lint everything?

generate only generates code. It does not run golangci-lint.

Generate is itself a lint because it verifies that the code used to generate the other code is correct... IIRC it also runs go vet which is itself a lint check.

There's no reason to separate lint into it's own check because there is no reason for the user to run lint on it's own. After they make changes they will need to generate, and that should flag if something is wrong that would prevent them from getting into a "valid state" to check in.

Fixed. All lint targets are now run as part of generate.

kurtisvg · 2022-11-07T02:34:03Z

operator.mk

+.PHONY: pr_check_lint
+pr_check_lint: git_workdir_clean lint # Used in the Github Action PR check to ensure generate does not introduce changes and lint passes
+	git diff --stat HEAD
+	@git diff --exit-code --stat HEAD || (echo ; echo ; echo "ERROR: Lint tools caused changes to the working dir. "; echo "       Please review the changes before you commit."; echo ; exit 1)
+	@echo "PR lint checks OK"


This seems like it could just happen in the workflow - does it need to be in the makefile?

My experience with software projects that are more than 10 years old is this: the build tool in the source (ant, mvn, make, npm, etc) has outlived several changes of the CI tool.

As a result, I have found that it is much easier to develop and maintain reliable validation and release processes run by CI when the most of the logic is written in the build tool.

I wrote all the steps needed to for the lint pr check into Makefile here. The lint.yaml is minimal: it simply calls make.

This makes it faster for me and any other developer to dial in the exact build process to check make, because I can test it my local dev machine. Also it reinforces the pattern for this project: build-related code is always in the Makefile.

There's already too much stuff in the Makefile - we need to keep it slim and avoid bloat so that it is maintainable and approachable. Putting too much stuff in a single place just adds indirection and means going down the rabbit hole to find where the actual logic lives.

It's also significantly more streamlined to put it in the workflow: It's one-line (make generate && git diff --exit-code) and makes it super clear what it's checking - it's running the generate step and throwing the error if there are any changes.

I agree that that the makefile is getting long and we may want to break it up for readability. I disagree with your analysis of the tradeoffs about which approach is more maintainable.

Why we need a separate 'generate' and 'lint' target:
The golangci-lint check is not included in the generate target because it takes ~45 seconds to run. We use generate all over the place and running a full golangci-lint on every build is unnecessary and slows the developer down.

Why put the CI lint check logic into the Makefile
For me, when I join on an old project with existing build and CI infrastructure, my first step to contributing is to figure out how to pass the PR checks. This is easiest when all the CI and build stuff in one language, in one place.

Generally if a PR check fails, I need to go digging the documentation and CI tooling to find answers to these questions:

What CI tool is building this project?

Do I have access to that CI tool?

What build actually failed?

What file controls that build, and how do I read that file?

What commands are being run?

How do I reproduce those commands on my laptop so I can figure out what is wrong?

In this PR, the lint CI job fails with an error that instructs the contributor run make pr_check_lint. All the interesting build logic used by the pr check is in the makefile. The new developer can easily figure out what is required of them in one file that they already know how to use.

If, as you propose, we start adding specific incantations of commands to the github workflow files like lint.yaml, then the developer has to go digging through those files, and we have to keep one more piece of information in the onboarding docs in sync with the code. In the long run, this will be harder to maintain than a makefile with an extra 4-line target in it.

The golangci-lint check is not included in the generate target because it takes ~45 seconds to run.

Golang CI is a combination of multiple linters, including goimports and go vet. It's claim to fame is that it's fast and does the lints using parallelism and caching. If it's really a significant amount of time to run, I think we should investigate if we need cache the earlier generation steps better so they aren't voiding the cache golangci-lint is using.

If, as you propose, we start adding specific incantations of commands to the github workflow files like lint.yaml, then the developer has to go digging through those files,

The developer doesn't need to understand how the presubmits work. It should clearly indicate what is wrong so that they can fix it. In the case of the linter, we should give them an error that says "run make generate to pass this check".

Fixed. lint is now part of generate and specific pr checks are now in the lint.yaml github action.

hessjcg force-pushed the release-lint branch from 7e4f07a to f7f430f Compare November 4, 2022 20:52

hessjcg marked this pull request as ready for review November 4, 2022 20:53

hessjcg requested review from a team, kurtisvg, enocom and shubha-rajan November 4, 2022 20:53

kurtisvg suggested changes Nov 7, 2022

View reviewed changes

hessjcg requested a review from kurtisvg November 7, 2022 16:02

hessjcg added 13 commits November 7, 2022 21:58

feat: add lint target to makefile

fff392f

feat: use the new makefile in the github pr checks.

cd58461

fix: correct documentation in pr_check_lint

7978363

fix: correct documentation in pr_check_lint

1e5a486

fix: kustomize version is now set correctly.

db50809

fix: improve makefile doc on tf_lint target.

fa8d5ac

fix: add the golangci_lint tools targets.

280bc12

fix: add GOOS and GOARCH so that the correct tools are installed.

06eac27

fix: make it easier to see what changed when lint is run.

442cc16

fix: upgraded version of controller-gen, causes lint error

e679c4c

fix: use operator.mk test target for github test

862b523

fix: rearrange steps in the lint.yaml workflow.

d3ccc55

fix: move lint pr check logic to github action.

2a60610

hessjcg force-pushed the release-lint branch from 0574008 to 2a60610 Compare November 8, 2022 04:58

kurtisvg approved these changes Nov 8, 2022

View reviewed changes

hessjcg merged commit 9f3d81b into main Nov 8, 2022

release-please bot mentioned this pull request Nov 8, 2022

chore(main): release 0.0.1 #85

Closed

hessjcg deleted the release-lint branch November 18, 2022 17:56

This was referenced Dec 7, 2022

chore(main): release 0.0.2 #131

Closed

chore(main): release 0.0.3 #134

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add lint target to makefile #101

feat: add lint target to makefile #101

hessjcg commented Nov 3, 2022 •

edited

Loading

kurtisvg Nov 7, 2022

hessjcg Nov 7, 2022

kurtisvg Nov 7, 2022

hessjcg Nov 7, 2022

kurtisvg Nov 7, 2022

hessjcg Nov 8, 2022

kurtisvg Nov 7, 2022

hessjcg Nov 7, 2022

kurtisvg Nov 7, 2022

hessjcg Nov 7, 2022 •

edited

Loading

kurtisvg Nov 7, 2022 •

edited

Loading

hessjcg Nov 8, 2022 •

edited

Loading

		- name: Verify no changes from lint tools. If you're reading this and the check has failed, run `make -f operator.mk pr_check_lint`.
		run: make -f operator.mk pr_check_lint

feat: add lint target to makefile #101

feat: add lint target to makefile #101

Conversation

hessjcg commented Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hessjcg Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

kurtisvg Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

hessjcg Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

hessjcg commented Nov 3, 2022 •

edited

Loading

hessjcg Nov 7, 2022 •

edited

Loading

kurtisvg Nov 7, 2022 •

edited

Loading

hessjcg Nov 8, 2022 •

edited

Loading