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.

Sign up for GitHub

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

Jump to bottom

Bug 1892472: Run linter, utests, and gofmt in container #428

Merged
merged 2 commits into from
Feb 1, 2021
Merged

Bug 1892472: Run linter, utests, and gofmt in container #428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
417414b
fix for the linter
JacobTanenbaum Jan 4, 2021
9c48e19
allows for running all tests inside a container (#1976)
vjayaramrh Jan 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 22 additions & 1 deletion go-controller/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,18 @@ PKGS ?=
GOPATH ?= $(shell go env GOPATH)
TEST_REPORT_DIR?=$(CURDIR)/_artifacts
export TEST_REPORT_DIR
GO_DOCKER_IMG = quay.io/giantswarm/golang:1.15.7
Copy link
Contributor

Choose a reason for hiding this comment

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

do we trust this?

Copy link
Contributor

Choose a reason for hiding this comment

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

The standard is to use golang:1.15-alpine, which I realize is DockerHub. We could make this overridable with a ?= and point to a CI-hosted image for automated runs.

Copy link
Contributor Author

@abhat abhat Feb 1, 2021 •
edited

Choose a reason for hiding this comment

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

Yeah, we can make this a downstream specific change. I think anything that is not hosted in Dockerhub which is relatively "official" will work.

Do you feel ok with merging this for now since this is a cherry-pick of upstream and I will create a downstream only PR to update that image to CI-hosted one?

Copy link
Contributor

Choose a reason for hiding this comment

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

it was mostly curiosity, not a blocking question :)

# CONTAINER_RUNNABLE determines if the tests can be run inside a container. It checks to see if
# podman/docker is installed on the system and also confirms that it is not running in a CI environment by checking
# that the GITHUB_ACTIONS environment variable is not set. NOTE: Running tests in a container inside of a CI environment
# fails some of the tests needing mount options inspite of starting the container with the right linux capabilities.
PODMAN ?= $(shell podman -v > /dev/null 2>&1; echo $$?)
ifeq ($(PODMAN), 0)
CONTAINER_RUNTIME=podman
else
CONTAINER_RUNTIME=docker
endif
CONTAINER_RUNNABLE ?= $(shell $(CONTAINER_RUNTIME) -v > /dev/null 2>&1 && ! printenv GITHUB_ACTIONS > /dev/null 2>&1; echo $$?)

.PHONY: all build check test

Expand All @@ -19,15 +31,20 @@ export TEST_REPORT_DIR
# (disables compiler optimization and inlining to aid source debugging tools
# like delve)


all build:
hack/build-go.sh cmd/ovnkube cmd/ovn-k8s-cni-overlay cmd/ovn-kube-util hybrid-overlay/cmd/hybrid-overlay-node cmd/ovndbchecker cmd/ovnkube-trace

windows:
WINDOWS_BUILD="yes" hack/build-go.sh hybrid-overlay/cmd/hybrid-overlay-node

# Note: `--security-opt label=disable` option is required on systems where SELinux is enabled for `podman` to successfully run the
# tests in a container. Refer: https://www.projectatomic.io/blog/2016/03/dwalsh_selinux_containers/ for additional context
check test:
ifeq ($(CONTAINER_RUNNABLE), 0)
$(CONTAINER_RUNTIME) run --security-opt label=disable --cap-add=NET_ADMIN --cap-add=SYS_ADMIN -v $(shell dirname $(PWD)):/go/src/github.com/ovn-org/ovn-kubernetes -w /go/src/github.com/ovn-org/ovn-kubernetes/go-controller $(GO_DOCKER_IMG) sh -c "RACE=1 DOCKER_TEST=1 hack/test-go.sh ${PKGS}"
else
RACE=1 hack/test-go.sh ${PKGS}
endif

codegen:
hack/update-codegen.sh
Expand All @@ -51,4 +68,8 @@ lint:
@GOPATH=${GOPATH} ./hack/lint.sh

gofmt:
ifeq ($(CONTAINER_RUNNABLE), 0)
$(CONTAINER_RUNTIME) run --security-opt label=disable -v $(shell dirname $(PWD)):/go/src/github.com/ovn-org/ovn-kubernetes -w /go/src/github.com/ovn-org/ovn-kubernetes/go-controller $(GO_DOCKER_IMG) hack/verify-gofmt.sh
else
@./hack/verify-gofmt.sh
endif
11 changes: 8 additions & 3 deletions go-controller/hack/lint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
#!/usr/bin/env bash
GO111MODULE=on ${GOPATH}/bin/golangci-lint run \
--timeout=15m0s --verbose --print-resources-usage --modules-download-mode=vendor \
&& echo "lint OK!"

# pin golangci-lint version to 1.33.2
VERSION=v1.33.2

docker run --rm -v $(pwd):/app -w /app -e GO111MODULE=on golangci/golangci-lint:${VERSION} \
golangci-lint run --verbose --print-resources-usage \
--modules-download-mode=vendor --timeout=15m0s && \
echo "lint OK!"
2 changes: 1 addition & 1 deletion go-controller/hack/test-go.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ function testrun {
if [ ! -z "${RACE:-}" ]; then
args="-race "
fi
if [[ "$USER" != root && " ${root_pkgs[@]} " =~ " $pkg " ]]; then
if [[ "$USER" != root && " ${root_pkgs[@]} " =~ " $pkg " && -z "${DOCKER_TEST:-}" ]]; then
testfile=$(mktemp --tmpdir ovn-test.XXXXXXXX)
echo "sudo required for ${pkg}, compiling test to ${testfile}"
if [[ ! -z "${RACE:-}" && "${pkg}" != "github.com/ovn-org/ovn-kubernetes/go-controller/hybrid-overlay/pkg/controller" ]]; then
Expand Down