Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
20 changes: 13 additions & 7 deletions .github/workflows/common.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,28 @@ jobs:
build:
runs-on: [self-hosted, macos, sequoia, ARM64]
timeout-minutes: 30
steps:
steps:
- uses: actions/setup-go@v5
with:
go-version: '1.23.2'
go-version: "1.23.2"
- name: Checkout repository
uses: actions/checkout@v4
uses: actions/checkout@v4
with:
fetch-depth: 0
- name: Check formatting
run: |
run: |
./scripts/install-hawkeye.sh
make fmt
git diff
if ! git diff --quiet ; then echo the following files require formatting or license headers: ; git diff --name-only ; false ; fi
- name: Check protobufs
run: |
- name: Verify linter configuration and Lint go code
uses: golangci/golangci-lint-action@v8

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.

I think we should instead call the makefile command to install golangci-lint and then run it. That way the behavior is the same between local dev and the CI run.

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.

Also the CI right now won't run for this PR right now since golangci/golangci-lint-action@v8 is not in our allowlist for actions that can be run

with:
verify: true
args: --timeout=30m
version: v2.1.6
- name: Check protobufs
run: |
make go-protos
if ! git diff --quiet ; then echo the following files require formatting or license headers: ; git diff --name-only ; false ; fi
- name: Build
Expand Down Expand Up @@ -77,4 +83,4 @@ jobs:
uses: actions/upload-artifact@v4
with:
name: container-builder-shim
path: container-builder-shim.tar
path: container-builder-shim.tar
114 changes: 114 additions & 0 deletions .golangci.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
version: "2"
linters:
default: none
settings:
misspell:
locale: US
govet:
enable:
- shadow
staticcheck:
checks:
[
"all",
"-ST1000",
"-ST1001",
"-ST1003",
"-ST1005",
"-ST1012",
"-ST1016",
"-ST1020",
"-ST1021",
"-ST1022",
"-QF1001",
"-QF1003",
"-QF1008",
]
exclusions:
generated: lax
presets: [comments, common-false-positives, legacy, std-error-handling]
rules:
- linters:
- dogsled
- errname
- exhaustive
- gomoddirectives
- govet
- interfacebloat
- musttag
- nakedret
- nilerr
- nilnil
- promlinter
text: .*
paths: [third_party, builtin$, examples$, vendor$]
warn-unused: true
enable:
- asasalint
- asciicheck
- bidichk
- bodyclose
- canonicalheader
- contextcheck
- copyloopvar
- decorder
- dogsled
- dupword
- durationcheck
- errcheck
- errchkjson
- errname
- exhaustive
- exptostd
- forbidigo
- ginkgolinter

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.

I'm not sure we need this. We don't have any ginkgo tests in this repo.

- gocheckcompilerdirectives
- gochecksumtype
- gocritic
- goheader
- goprintffuncname
- gosec
- gosmopolitan
- govet
- grouper
- importas
- ineffassign
- interfacebloat
- intrange
- loggercheck
- makezero
- mirror
- misspell
- musttag
- nakedret
- nilerr
- nilnil
- noctx
- nosprintfhostport
- predeclared
- promlinter
- protogetter
- reassign
- sloglint
- staticcheck
- tagalign
- testableexamples
- unconvert
- unparam
- unused
- usestdlibvars
- usetesting
- wastedassign
formatters:
enable: [gci, gofmt]
settings:
gci:
sections:
- standard
- defaul

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.

default

- prefix(github.com/apple/container-builder-shim)
- blank
- dot
exclusions:
generated: lax
paths: [third_party, builtin$, examples$]
19 changes: 11 additions & 8 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -50,19 +50,23 @@ fmt: go-fmt update-licenses

.PHONY: go-fmt
go-fmt:
$(GO) fmt $(PKG)
$(GO) fmt -l -s -w $(PKG)

.PHONY: vet
vet:
$(GO) vet $(PKG)

golangci-lint-install:
@scripts/install-tools.sh --golangci

.PHONY: go-lint
go-lint: golangci-lint-install
$(GOLANGCI_LINT) config verify

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.

I don't think this will work when the script has installed golangci-lint under .local/bin. I think following something similar to what we do for installing protoc would be good. What do you think?

gofmt -l -s -w $(PKG)
$(GOLANGCI_LINT) run --timeout=30m --fix ./...

.PHONY: lint
lint:
ifndef GOLANGCI_LINT
@echo "golangci-lint not found – skipping. Install from https://golangci-lint.run/welcome/install/ to enable." >&2
else
$(GOLANGCI_LINT) run
endif
lint: go-lint

.PHONY: tidy
tidy:
Expand Down Expand Up @@ -117,4 +121,3 @@ release: fmt vet lint test build-linux docker-image
clean:
$(GO) clean
rm -rf $(BUILD_DIR) coverage.out

49 changes: 49 additions & 0 deletions scripts/install-tools.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
#!/usr/bin/env bash
# Copyright © 2025 Apple Inc. and the container-builder-shim project authors. All rights reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# https://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

GOLANGCI_LINTER_VERSION=v2.1.6

install_golangci() {
ci_install=false
if [[ -x $(which golangci-lint) ]]; then

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.

I think which will not find the binary if it's installed under .local/bin

local version=$(golangci-lint version --short)
if [[ "${version}" == "${GOLANGCI_LINTER_VERSION#v}" ]]; then
ci_install=false
else
ci_install=true
Comment on lines +22 to +25

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.

If we do something like how we install protoc, we get the check for whether to install golangci-lint for free since make will check if the file path exists, as it does for protoc here. Does that make sense?

fi
else
ci_install=true
fi
if [[ "$ci_install" == true ]]; then
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/cc3567e3127d8530afb69be1b7bd20ba9ebcc7c1/install.sh \

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.

In the other container project repos, we tend to install things under a .local directory for make to use (example https://github.com/apple/container/blob/9eb2f860fc35a933e7894bc5313e3b79cd001c52/Makefile#L162).

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.

I think we could do something like this for the address

https://raw.githubusercontent.com/golangci/golangci-lint/${GOLANGCI_LINTER_VERSION}/install.sh

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yup that makes sense.. changing it

| sh -s -- -b $(go env GOPATH)/bin "${GOLANGCI_LINTER_VERSION}"
fi
}

function main() {
case $1 in
--golangci)
install_golangci
;;
*)
echo "unknown command" >&2
show_help
Comment thread
katiewasnothere marked this conversation as resolved.
exit 1
;;
esac
}

main "$@"