-
Notifications
You must be signed in to change notification settings - Fork 57
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
adds support for unit tests, golangci-lint and gosec #83
Conversation
vjayaramrh
commented
Apr 1, 2023
- adds support for running unit tests inside of containers, generates html output file to visualize code coverage by tests
- adds support for running golangci-lint inside of containers (https://golangci-lint.run/)
- adds support for running gosec inside of containers (https://github.com/securego/gosec)
|
@vjayaramrh @radoslawc can you please take care of this PR as well while creating prow jobs for this repo? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vjayaramrh I tried to run it but I got the following error:
$ make unit
Makefile:20: *** missing separator (did you mean TAB instead of 8 spaces?). Stop.
After replacing the spaces with tabs, I was eable to run it.
diff --git a/Makefile b/Makefile
index cf282a5..6950216 100644
--- a/Makefile
+++ b/Makefile
@@ -17,37 +17,37 @@ CONTAINER_RUNNABLE ?= $(shell $(CONTAINER_RUNTIME) -v > /dev/null 2>&1; echo $$?
.PHONY: unit_clean
unit_clean: ## clean up the unit test artifacts created
ifeq ($(CONTAINER_RUNNABLE), 0)
- $(CONTAINER_RUNTIME) system prune -f
+ $(CONTAINER_RUNTIME) system prune -f
endif
- rm ${TEST_COVERAGE_FILE} ${TEST_COVERAGE_HTML_FILE} ${TEST_COVERAGE_FUNC_FILE} > /dev/null 2>&1
+ rm ${TEST_COVERAGE_FILE} ${TEST_COVERAGE_HTML_FILE} ${TEST_COVERAGE_FUNC_FILE} > /dev/null 2>&1
.PHONY: unit
unit: ## Run unit tests against code.
ifeq ($(CONTAINER_RUNNABLE), 0)
- $(CONTAINER_RUNTIME) run -it -v ${PWD}:/go/src -w /go/src docker.io/library/golang:${GO_VERSION}-alpine3.17 \
- /bin/sh -c "go test ./... -v -coverprofile ${TEST_COVERAGE_FILE}; \
- go tool cover -html=${TEST_COVERAGE_FILE} -o ${TEST_COVERAGE_HTML_FILE}; \
- go tool cover -func=${TEST_COVERAGE_FILE} -o ${TEST_COVERAGE_FUNC_FILE}"
+ $(CONTAINER_RUNTIME) run -it -v ${PWD}:/go/src -w /go/src docker.io/library/golang:${GO_VERSION}-alpine3.17 \
+ /bin/sh -c "go test ./... -v -coverprofile ${TEST_COVERAGE_FILE}; \
+ go tool cover -html=${TEST_COVERAGE_FILE} -o ${TEST_COVERAGE_HTML_FILE}; \
+ go tool cover -func=${TEST_COVERAGE_FILE} -o ${TEST_COVERAGE_FUNC_FILE}"
else
- go test ./... -v -coverprofile ${TEST_COVERAGE_FILE}
- go tool cover -html=${TEST_COVERAGE_FILE} -o ${TEST_COVERAGE_HTML_FILE}
- go tool cover -func=${TEST_COVERAGE_FILE} -o ${TEST_COVERAGE_FUNC_FILE}
+ go test ./... -v -coverprofile ${TEST_COVERAGE_FILE}
+ go tool cover -html=${TEST_COVERAGE_FILE} -o ${TEST_COVERAGE_HTML_FILE}
+ go tool cover -func=${TEST_COVERAGE_FILE} -o ${TEST_COVERAGE_FUNC_FILE}
endif
# Install link at https://golangci-lint.run/usage/install/ if not running inside a container
.PHONY: lint
lint: ## Run lint against code.
ifeq ($(CONTAINER_RUNNABLE), 0)
- $(CONTAINER_RUNTIME) run -it -v ${PWD}:/go/src -w /go/src docker.io/golangci/golangci-lint:${GOLANG_CI_VER}-alpine golangci-lint run ./... -v
+ $(CONTAINER_RUNTIME) run -it -v ${PWD}:/go/src -w /go/src docker.io/golangci/golangci-lint:${GOLANG_CI_VER}-alpine golangci-lint run ./... -v
else
- golangci-lint run ./... -v
+ golangci-lint run ./... -v
endif
# Install link at https://github.com/securego/gosec#install if not running inside a container
.PHONY: gosec
gosec: ## inspects source code for security problem by scanning the Go Abstract Syntax Tree
ifeq ($(CONTAINER_RUNNABLE), 0)
- $(CONTAINER_RUNTIME) run -it -v ${PWD}:/go/src -w /go/src docker.io/securego/gosec:${GOSEC_VER} ./...
+ $(CONTAINER_RUNTIME) run -it -v ${PWD}:/go/src -w /go/src docker.io/securego/gosec:${GOSEC_VER} ./...
else
- gosec ./...
+ gosec ./...
endif
I couldn't add the suggestion into the PR, but those are the changes. BTW, I got failures given this repo doesn't contain go lang source so I guess that's normal
I have fixed the space vs TAB issue, would appreciate if you can confirm it is resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
@electrocucaracha: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/assign @johnbelamaric |
I don't have Podman installed and got this error: podman run -it -v /Users/henderiw/code/nephio/nephio-api:/go/src -w /go/src docker.io/library/golang:-alpine3.17 |
@henderiw Good to know, let me investigate |
@henderiw I believe that you may have podman installed, but the podman machine is not running. If the podman machine is stopped, you can restart it using the command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tools don't always put newlines at the end of files, can we see if that's a setting or something?
@henderiw: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Downloaded this PR and ran the Make unit on my MAC/Docker Desktop, and it ran successfully. /lgtm |
/approve |
@vishwanathj looks like the tab issue is still there. Perhaps the commit was never pushed? |
strange, let me investigate |
/retest |
@vjayaramrh: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
@vjayaramrh: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/lgtm |
@electrocucaracha: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Testing after signing CLA /lgtm |
@electrocucaracha: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/lgtm |
@henderiw: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
.PHONY: unit_clean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to change Makefile commands you have to change PROW job as well. So for example you want to add 'make unit_clean' you have to add it in .prow.yaml for example like this:
- name: presubmit-nephio-go-unit-clean
decorate: true
run_if_changed: "(\\.go)$"
spec:
containers:
- image: golang:1.20.2
command:
- make
args:
- unit_clean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@radoslawc Thanks, I shall update the PR
/test ? |
@radoslawc: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
Makefile
Outdated
go tool cover -func=${TEST_COVERAGE_FILE} -o ${TEST_COVERAGE_FUNC_FILE}" | ||
else | ||
go test ./... -v -coverprofile ${TEST_COVERAGE_FILE} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like there are whitespaces there instead of tabs
/ok-to-test |
Makefile
Outdated
$(CONTAINER_RUNTIME) run -it -v ${PWD}:/go/src -w /go/src docker.io/golangci/golangci-lint:${GOLANG_CI_VER}-alpine golangci-lint run ./... -v | ||
else | ||
golangci-lint run ./... -v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
golangci-lint binary has to be installed in image used in .prow.yaml for example:
- name: presubmit-nephio-golangci-lint
decorate: true
run_if_changed: "(\\.go)$"
spec:
containers:
- image: golang:1.20.2
command:
- "/bin/sh"
- "-c"
- |
wget -O- -nv https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s v1.52.2
make lint
otherwise running it in PROW job will end up with error:
make: golangci-lint: No such file or directory
Makefile
Outdated
$(CONTAINER_RUNTIME) run -it -v ${PWD}:/go/src -w /go/src docker.io/securego/gosec:${GOSEC_VER} ./... | ||
else | ||
gosec ./... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gosec binary has to be installed in image used in .prow.yaml for example:
- name: presubmit-nephio-gosec
decorate: true
run_if_changed: "(\\.go)$"
spec:
containers:
- image: golang:1.20.2
command:
- "/bin/sh"
- "-c"
- |
wget -O - -q https://raw.githubusercontent.com/securego/gosec/master/install.sh | sh -s v2.15.0
make gosec
otherwise running it in PROW job will end up with error:
make: gosec: No such file or directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@radoslawc I see your comment about gosec here
cc: @johnbelamaric
We could build docker image with gosec and golangci-lint based on golang alpine image that we're using. In that way we could have uniform test across all repositories (granted people will use our image). |
/approve @radoslawc yes, we probably need some sort of base image for builds as well as for our functions, etc. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: electrocucaracha, henderiw, johnbelamaric, rravindran123, vjayaramrh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Do we need to build or use the images already published by those projects on dockerhub? |