Skip to content

GitHub Actions for lint - #3211

Merged
silvin-lubecki merged 1 commit into
docker:masterfrom
crazy-max:gha
Aug 5, 2021
Merged

GitHub Actions for lint#3211
silvin-lubecki merged 1 commit into
docker:masterfrom
crazy-max:gha

Conversation

@crazy-max

@crazy-maxcrazy-max commented Jul 26, 2021

Copy link
Copy Markdown
Member

First step to switch from CircleCI to GitHub Actions for lint.

shellcheck and lint Dockerfiles have also been simplified and don't need to be built and run.

Have also added new bake targets for that:

  • docker buildx bake lint or make -f docker.Makefile lint
  • docker buildx bake shellcheck or make -f docker.Makefile shellcheck

cc @thaJeztah

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

@crazy-max
crazy-maxforce-pushed the gha branch 2 times, most recently from f1f3394 to 2580deaCompareJuly 26, 2021 15:25
@crazy-max
crazy-max marked this pull request as ready for review July 26, 2021 15:31
@crazy-max

Copy link
Copy Markdown
MemberAuthor

We could also cleanup docker.Makefile in a follow-up.

Comment threadMakefile Outdated
Comment threadMakefile Outdated
Comment threaddockerfiles/Dockerfile.shellcheck
@codecov-commenter

codecov-commenter commented Jul 26, 2021

Copy link
Copy Markdown

Codecov Report

Merging #3211 (698c155) into master (13e4a09) will not change coverage.
The diff coverage is n/a.

@@ Coverage Diff @@## master #3211 +/- ##
=======================================
Coverage 58.58% 58.58% =======================================
Files 299 299 Lines 21502 21502 =======================================
Hits 12597 12597 Misses 7983 7983 Partials 922 922 

@crazy-max
crazy-max requested a review from thaJeztahJuly 26, 2021 19:23
Comment threaddockerfiles/Dockerfile.lint Outdated
Comment threaddockerfiles/Dockerfile.lint Outdated
Comment threaddockerfiles/Dockerfile.shellcheck Outdated
Comment threaddockerfiles/Dockerfile.shellcheck
Comment threaddocker-bake.hcl Outdated
Comment threaddocker.Makefile Outdated
Comment threaddocker.Makefile Outdated
@thaJeztah

Copy link
Copy Markdown
Member

Whoops; this needs a rebase, because the go1.16 update was merged

@crazy-max

Copy link
Copy Markdown
MemberAuthor

Whoops; this needs a rebase, because the go1.16 update was merged

Ah yes indeed, will do that

@crazy-max
crazy-maxforce-pushed the gha branch 2 times, most recently from 911c49e to ca626e1CompareJuly 29, 2021 12:09
@crazy-max

Copy link
Copy Markdown
MemberAuthor

@thaJeztah After rebase looks like there are some warnings with golangci-lint which are not visible on CircleCI:

#16 [lint 1/1] RUN --mount=type=bind,target=.,rw --mount=type=cache,target=/root/.cache/go-build --mount=type=cache,target=/root/.cache/golangci-lint --mount=from=golangci-lint,source=/usr/bin/golangci-lint,target=/usr/bin/golangci-lint golangci-lint run --config=.golangci.yml
#16 85.79 level=warning msg="[runner] Can't run linter unused: buildssa: analysis skipped: errors in package: [/go/src/github.com/docker/cli/cli/config/credentials/default_store.go:4:7: could not import golang.org/x/sys/execabs (/go/src/github.com/docker/cli/vendor/golang.org/x/sys/execabs/execabs.go:17:2: could not import context (/usr/local/go/src/context/context.go:51:2: could not import errors (/usr/local/go/src/errors/wrap.go:8:2: could not import internal/reflectlite (/usr/local/go/src/internal/reflectlite/swapper.go:8:2: could not import internal/unsafeheader (-: could not load export data: cannot import \"internal/unsafeheader\" (unknown iexport format version 1), export data is newer version - update tool))))) /go/src/github.com/docker/cli/cli/config/credentials/default_store_linux.go:4:2: could not import os/exec (/usr/local/go/src/os/exec/exec.go:24:2: could not import bytes (/usr/local/go/src/bytes/buffer.go:10:2: could not import errors (/usr/local/go/src/errors/wrap.go:8:2: could not import internal/reflectlite (/usr/local/go/src/internal/reflectlite/swapper.go:8:2: could not import internal/unsafeheader (-: could not load export data: cannot import \"internal/unsafeheader\" (unknown iexport format version 1), export data is newer version - update tool))))) /go/src/github.com/docker/cli/cli/config/credentials/file_store.go:4:2: could not import strings (/usr/local/go/src/strings/builder.go:8:2: could not import unicode/utf8 (-: could not load export data: cannot import \"unicode/utf8\" (unknown iexport format version 1), export data is newer version - update tool)) /go/src/github.com/docker/cli/cli/config/credentials/native_store.go:5:2: could not import github.com/docker/docker-credential-helpers/client (/go/src/github.com/docker/cli/vendor/github.com/docker/docker-credential-helpers/client/client.go:4:2: could not import bytes (/usr/local/go/src/bytes/buffer.go:10:2: could not import errors (/usr/local/go/src/errors/wrap.go:8:2: could not import internal/reflectlite (/usr/local/go/src/internal/reflectlite/swapper.go:8:2: could not import internal/unsafeheader (-: could not load export data: cannot import \"internal/unsafeheader\" (unknown iexport format version 1), export data is newer version - update tool))))) /go/src/github.com/docker/cli/cli/config/credentials/native_store.go:6:2: could not import github.com/docker/docker-credential-helpers/credentials (/go/src/github.com/docker/cli/vendor/github.com/docker/docker-credential-helpers/credentials/credentials.go:4:2: could not import bufio (/usr/local/go/src/bufio/bufio.go:11:2: could not import bytes (/usr/local/go/src/bytes/buffer.go:10:2: could not import errors (/usr/local/go/src/errors/wrap.go:8:2: could not import internal/reflectlite (/usr/local/go/src/internal/reflectlite/swapper.go:8:2: could not import internal/unsafeheader (-: could not load export data: cannot import \"internal/unsafeheader\" (unknown iexport format version 1), export data is newer version - update tool))))))]"
#16 86.33 level=warning msg="[runner] Can't run linter goanalysis_metalinter: assign: failed prerequisites: inspect@github.com/docker/cli/cli/context"

Looks linked to golangci/golangci-lint#885. 1.23.8 fixed the issue.

Comment thread.github/workflows/validate.yml Outdated
Comment thread.github/workflows/validate.yml
fail-fast: false
matrix:
target:
- lint

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The official golangci-lint GitHub Action can also run on Windows, Darwin. Wondering if we can trick it by setting GOOS/GOARCH to also lint non-matching platforms (especially for the CLI, which will be extensively used on macOS and Windows)

@crazy-maxcrazy-maxAug 5, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes I think we can have a special matrix to run Windows and MacOS runners in this case. We can do that in follow-up.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, it's tricky, because of course we can't run a macOS container, and Windows doesn't support BuildKit for Windows containers (if it would, it's still possible that building takes a long time (Moby Ci can take 10 minutes to build the Dockerfile, a large portion of that is taken by just pulling the base image 😞).

So if it's possible to somehow trick golangci-lint (through GOOS / GOARCH) that would be a lot easier.

If not, then perhaps (for CI) we would have to consider using the official golangci-lint GitHub action, which doesn't (I think?) run containerised, but at least runs fast (and on Windows and macOS). We can still use the BuildKit approach for local use (and for Linux perhaps).

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@thaJeztah

using the official golangci-lint GitHub action

Yes I think we can do that with https://github.com/golangci/golangci-lint-action for Win/MacOS.

Comment threaddocker.Makefile Outdated
Comment threaddockerfiles/Dockerfile.lint Outdated
Comment threaddockerfiles/Dockerfile.shellcheck Outdated
Comment threaddockerfiles/Dockerfile.shellcheck Outdated
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

@thaJeztahthaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks!

@thaJeztah

Copy link
Copy Markdown
Member

@silvin-lubecki PTAL

@silvin-lubeckisilvin-lubecki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, thanks @crazy-max !

@silvin-lubecki
silvin-lubecki merged commit aa949f2 into docker:masterAug 5, 2021
@crazy-max
crazy-max deleted the gha branch August 5, 2021 12:12
@thaJeztahthaJeztah added this to the 21.xx milestone Aug 5, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@crazy-max@codecov-commenter@thaJeztah@silvin-lubecki