Use "Check Go" template workflow and related tasks #213
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `go:check-formatting` task has no value to the developer, since they will be better off to just run the formatting task. This means that the only use for this task is the CI. I think this can be better achieved by configuring the CI to format the code and then check for a diff. This ensures that there is no possibility for a mismatch between the formatting check and the formatting task.
per1234
added
type: enhancement
Since `go fmt` is only a formatter, it was necessary to use `gofmt` for the formatting check task, since that command has an option to skip writing the code. In order to ensure consistency between the two, I also used `gofmt` for the formatting task. The unfortunate thing about gofmt is that it works from a list of paths, rather than a list of packages as is the case with `go vet`, `go fix`, and `golint`. With the removal of the superfluous formatting check task, the motivation for using gofmt in the formatting task is removed, freeing me to use the superior `go fmt`.
GitHub Actions provides a paths filter feature that allows you to restrict workflow run triggers to changes to specific paths within the repository. This can greatly improve the efficiency of the CI system by avoiding running irrelevant checks. The effective use of this feature requires that the purpose of a workflow be focused on a specific type of file or project component. The "Check Formatting" workflow was the antithesis of that; grouping formatting checks of all types of files.
Splitting a workflow into distinct jobs (and steps within those jobs in the case of operations that need to occur in the same environment) makes interpreting a CI failure much easier. The check results are displayed on a per-job basis, each job having its own logs, and the results of that job shown on a per-step basis, each step having its own folded section in the logs. This means you can quickly identify which part of the workflow failed and get the logs specific to that.
It seems best to use the same version of Go when linting the code in the CI workflow runs as we use when testing and building that code. Previously, whichever version of Go that happened to be installed in the runner at the time was used.
Running the `go get` or `go list` commands from the module folder results in a change to the `go.mod` file. That change is reverted when you run `go mod tidy`. This results in cryptic diffs that can be confusing to contributors. The problem is avoided by running the commands from a separate folder.
The`GOLINTFLAGS` variable is only used in a single place, so it doesn't provide any benefit from the standpoint of code duplication. I don't feel that the taskfile usability is improved at all by defining these flags in a location far removed from the command they are used with, and it makes the taskfile more cryptic to contributors who are not experienced with taskfiles. So I think it's better to simply remove this variable.
GitHub Actions provides a paths filter feature that allows you to restrict workflow run triggers to changes to specific paths within the repository. This can greatly improve the efficiency of the CI system by avoiding running irrelevant checks. The effective use of this feature requires that the purpose of a workflow be focused on a specific type of file or project component. Even though the "Lint Code" workflow was in effect focused only on Go code, it's stated purpose was for the linting of all code. The "Check Go" workflow is collects the linting and formatting checks specific to the project's Go files.
This command moderizes usages of outdated Go APIs. If usage of any such APIs are found, the workflow job will fail, showing a diff of the suggested changes in the logs.
Changes to the go.mod manifest file or go.sum lock files can be unexpected and cryptic. The contributor may be confused about whether to commit these to the repository, or may commit unwanted changes, causing confusion to the next contributor when they get a mysterious diff. To avoid this, a check is added to the "Check Go" CI workflow which fails the job if a run of `go mod tidy` results in any diff.
This will make it easier for the maintainers to sync fixes and improvements in either direction between the upstream "template" tasks and their installation in this repository.
This is the variable name used by the Tooling team's template tasks.
The incorrect variable name used in the `go:lint` task's `golint` command resulted in the task doing nothing, meaning that `golint` has never been ran on the project's packages.
These violations were missed due to the error in the `golint` command used by the `go:lint` task.
This list is used for linting and formatting checks, which don't apply to tool generated code.
Codecov Report
@@ Coverage Diff @@
## main #213 +/- ##
=======================================
Coverage 87.97% 87.97%
=======================================
Files 43 43
Lines 4176 4176
=======================================
Hits 3674 3674
Misses 391 391
Partials 111 111
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
umbynos
approved these changes
Jul 28, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Source:
https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/check-go-task.md