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

Reorganise the magefile #11

Merged
merged 1 commit into from
Feb 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .ci/Jenkinsfile
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ pipeline {
withGithubNotify(context: "Lint") {
dir("${BASE_DIR}"){
withMageEnv(){
sh(label: 'Mage check', script: 'mage check')
sh(label: 'Mage check', script: 'mage -v check')
Copy link
Member Author

@rdner rdner Feb 18, 2022

Choose a reason for hiding this comment

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

It's helpful for diagnosing problems on the CI

}
}
}
Expand Down
10 changes: 10 additions & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,16 @@ run:
# timeout for analysis, e.g. 30s, 5m, default is 1m
timeout: 1m

issues:
# Maximum count of issues with the same text.
# Set to 0 to disable.
# Default: 3
max-same-issues: 0
# Maximum issues count per one linter.
# Set to 0 to disable.
# Default: 50
max-issues-per-linter: 0
Copy link
Member Author

@rdner rdner Feb 18, 2022

Choose a reason for hiding this comment

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

I personally prefer to see the entire list of issues at once and work my way through them without re-running the linter all the time.


output:
sort-results: true

Expand Down
132 changes: 105 additions & 27 deletions magefile.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,81 +36,137 @@ import (
)

const (
linterInstallFilename = "./intall-golang-ci.sh"
linterBinaryFilename = "./bin/golangci-lint"
linterInstallURL = "https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh"
linterInstallFilename = "./build/intall-golang-ci.sh"
linterBinaryFilename = "./build/golangci-lint"
linterVersion = "v1.44.0"
)

// InstallLinter installs golangci-lint (https://golangci-lint.run) to `./bin`
// Aliases are shortcuts to long target names.
// nolint: deadcode // it's used by `mage`.
var Aliases = map[string]interface{}{
"llc": Linter.LastChange,
"lint": Linter.All,
}

// Linter contains targets related to linting the Go code
type Linter mg.Namespace

// Install installs golangci-lint (https://golangci-lint.run) to `./build`
// using the official installation script downloaded from GitHub.
// If the linter binary already exists does nothing.
func InstallLinter() error {
_, err := os.Stat(linterBinaryFilename)
func (Linter) Install() error {
dirPath := filepath.Dir(linterBinaryFilename)
err := os.MkdirAll(dirPath, 0700)
if err != nil {
return fmt.Errorf("failed to create path %q: %w", dirPath, err)
}

_, err = os.Stat(linterBinaryFilename)
if err == nil {
log.Println("already installed, exiting...")
log.Println("the linter has been already installed, skipping...")
return nil
}
if !errors.Is(err, os.ErrNotExist) {
return err
return fmt.Errorf("failed check if file %q exists: %w", linterBinaryFilename, err)
}

log.Println("preparing the installation script file...")

installScript, err := os.OpenFile(linterInstallFilename, os.O_TRUNC|os.O_CREATE|os.O_WRONLY, 0700)
if err != nil {
return err
return fmt.Errorf("failed to create file %q: %w", linterInstallFilename, err)
}
defer installScript.Close()

log.Println("downloading the linter installation script...")
resp, err := http.Get("https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh")
// nolint: noctx // valid use since there is no context
resp, err := http.Get(linterInstallURL)
if err != nil {
return err
return fmt.Errorf("cannot download the linter installation script from %q: %w", linterInstallURL, err)
}
defer resp.Body.Close()

lr := io.LimitReader(resp.Body, 1024*100) // not more than 100 KB
lr := io.LimitReader(resp.Body, 1024*100) // not more than 100 KB, just to be safe
_, err = io.Copy(installScript, lr)
if err != nil {
return err
return fmt.Errorf("failed to finish downloading the linter installation script: %w", err)
}

err = installScript.Close() // otherwise we cannot run the script
if err != nil {
return err
return fmt.Errorf("failed to close file %q: %w", linterInstallFilename, err)
}

binaryDir := filepath.Dir(linterBinaryFilename)
err = os.MkdirAll(binaryDir, 0700)
if err != nil {
return fmt.Errorf("cannot create path %q: %w", binaryDir, err)
}

return sh.Run(linterInstallFilename, linterVersion)
// there must be no space after `-b`, otherwise the script does not work correctly ¯\_(ツ)_/¯
return sh.Run(linterInstallFilename, "-b"+binaryDir, linterVersion)
}

// LintAll runs the linter against the entire codebase
func LintAll() error {
mg.Deps(InstallLinter)
return sh.Run(linterBinaryFilename, "-v", "run", "./...")
// All runs the linter against the entire codebase
func (l Linter) All() error {
mg.Deps(l.Install)
return runLinter()
}

// LastChange runs the linter against all files changed since the fork point from `main`.
// If the current branch is `main` then runs against the files changed in the last commit.
func (l Linter) LastChange() error {
mg.Deps(l.Install)

branch, err := sh.Output("git", "branch", "--show-current")
if err != nil {
return fmt.Errorf("failed to get the current branch: %w", err)
}

// the linter is supposed to support linting changed diffs only but,
// for some reason, it simply does not work - does not output any
// results without linting the whole files, so we have to use `--whole-files`
// which can lead to some frustration from developers who would like to
// fix a single line in an existing codebase and the linter would force them
// into fixing all linting issues in the whole file instead

if branch == "main" {
// files changed in the last commit
return runLinter("--new-from-rev=HEAD~", "--whole-files")
}

return runLinter("--new-from-rev=origin/main", "--whole-files")
}

// Check runs all the checks
// nolint: deadcode,unparam // it's used as a `mage` target and requires returning an error
func Check() error {
mg.Deps(CheckNoBeatsDependency, CheckModuleTidy, LintAll)
mg.Deps(Deps.CheckNoBeats, Deps.CheckModuleTidy, Linter.LastChange)
return nil
}

// CheckNoBeatsDependency is required to make sure we are not introducing
// Deps contains targets related to checking dependencies
type Deps mg.Namespace

// CheckNoBeats is required to make sure we are not introducing
// dependency on elastic/beats.
func CheckNoBeatsDependency() error {
func (Deps) CheckNoBeats() error {
goModPath, err := filepath.Abs("go.mod")
if err != nil {
return err
}
goModFile, err := os.Open(goModPath)
if err != nil {
return fmt.Errorf("failed to open module file: %+v", err)
return fmt.Errorf("failed to open module file: %w", err)
}
beatsImport := []byte("github.com/elastic/beats")
scanner := bufio.NewScanner(goModFile)
lineCount := 1
for scanner.Scan() {
line := scanner.Bytes()
if bytes.Contains(line, beatsImport) {
return fmt.Errorf("line %d is beats dependency: '%s'\nPlease, make sure you are not adding anything that depends on %s", lineCount, line, beatsImport)
return fmt.Errorf("line %d is a beats dependency: '%s'\nPlease, make sure you are not adding anything that depends on %s", lineCount, line, beatsImport)
}
lineCount++
}
Expand All @@ -120,16 +176,38 @@ func CheckNoBeatsDependency() error {
return nil
}

// CheckModuleTidy checks if `go mod tidy` was run before.
func CheckModuleTidy() error {
// CheckModuleTidy checks if `go mod tidy` was run before the last commit.
func (Deps) CheckModuleTidy() error {
err := sh.Run("go", "mod", "tidy")
if err != nil {
return err
}
err = sh.Run("git", "diff", "--exit-code")
err = sh.Run("git", "diff", "--exit-code", "go.mod")
Copy link
Member Author

Choose a reason for hiding this comment

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

This was a bug, any change in the repo caused the failed check and this misleading error message.

if err != nil {
return fmt.Errorf("`go mod tidy` was not called before committing: %w", err)
return fmt.Errorf("`go mod tidy` was not called before the last commit: %w", err)
}

return nil
}

// runWithStdErr runs a command redirecting its stderr to the console instead of discarding it
func runWithStdErr(command string, args ...string) error {
_, err := sh.Exec(nil, os.Stdout, os.Stderr, command, args...)
return err
}

// runLinter runs the linter passing the `mage -v` (verbose mode) and given arguments.
// Also redirects linter's output to the `stderr` instead of discarding it.
func runLinter(runFlags ...string) error {
var args []string

if mg.Verbose() {
args = append(args, "-v")
}

args = append(args, "run")
args = append(args, runFlags...)
args = append(args, "./...")

return runWithStdErr(linterBinaryFilename, args...)
}