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

allows for running all tests inside a container #1976

Merged
merged 2 commits into from
Jan 28, 2021

Conversation

vjayaramrh
Copy link
Collaborator

@vjayaramrh vjayaramrh commented Jan 22, 2021

Co-authored-by: Aniket Bhat anbhat@redhat.com
Signed-off-by: Vishwanath Jayaraman vjayaram@redhat.com

- What this PR does and why is it needed

This PR adds the ability to run tests (including those requiring sudo such as tests in pkg/node and hybrid-overlay/pkg/controller) inside of a golang 1.15 docker container. Allows for running tests on non-linux systems as well.

- Special notes for reviewers

- How to verify it

Execute make check or make test on your development system that has docker installed from the ovn-kuberentes/go-controller directory.

- Description for the changelog

@coveralls
Copy link

coveralls commented Jan 22, 2021

Coverage Status

Coverage decreased (-0.5%) to 54.666% when pulling ab4130f on vjayaramrh:dockerize_tests into 9c1fca2 on ovn-org:master.

@abhat
Copy link
Contributor

abhat commented Jan 22, 2021

why not make this replace the check and test targets?

@vjayaramrh
Copy link
Collaborator Author

why not make this replace the check and test targets?

Allows for the choice of running tests on systems where docker is not installed. However, if there is a consensus, I can implement your suggestion.

@girishmg
Copy link
Member

girishmg commented Jan 24, 2021

@vjayaramrh this looks very good as is. I am going to start using this on my Macbook once this integrates.

Can you please also add make, make lint, and make gofmt?

@abhat as a good point. I am fine either ways not sure what @dcbw @trozet think?

@aojea will it have any impact in Github's ci/cd?

@aojea
Copy link
Contributor

aojea commented Jan 24, 2021

@aojea will it have any impact in Github's ci/cd?

from the top of my head it shouldn't

why not make this replace the check and test targets?

You should keep an option for people without docker/podman installed, I think that just checking if docker exists should be enough

This is really nice, so we have consistency +1 to making it the default

@vjayaramrh
Copy link
Collaborator Author

vjayaramrh commented Jan 24, 2021

@vjayaramrh this looks very good as is. I am going to start using this on my Macbook once this integrates.

Can you please also add make, make lint, and make gofmt?

@abhat as a good point. I am fine either ways not sure what @dcbw @trozet think?

@aojea will it have any impact in Github's ci/cd?

@girishmg Looks like hack/lint.sh is already invoking a docker container, let me see what I can do in this PR related to make gofmt

@vjayaramrh
Copy link
Collaborator Author

@aojea will it have any impact in Github's ci/cd?

from the top of my head it shouldn't

why not make this replace the check and test targets?

You should keep an option for people without docker/podman installed, I think that just checking if docker exists should be enough

This is really nice, so we have consistency +1 to making it the default

@aojea let me see if I can do a check for docker installed and keep the same targets as suggested by @abhat

@vjayaramrh vjayaramrh force-pushed the dockerize_tests branch 5 times, most recently from 95a4bf0 to 1608aea Compare January 24, 2021 22:20
go-controller/Makefile Outdated Show resolved Hide resolved
@aojea
Copy link
Contributor

aojea commented Jan 25, 2021

/lgtm
Thanks

@@ -9,6 +9,12 @@ PKGS ?=
GOPATH ?= $(shell go env GOPATH)
TEST_REPORT_DIR?=$(CURDIR)/_artifacts
export TEST_REPORT_DIR
GO_DKR_IMG = golang:1.15.7-buster
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: GO_DOCKER_IMG instead of GO_DKR_IMG.
Also, what is the need for a buster image?

Copy link
Contributor

Choose a reason for hiding this comment

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

As an aside, I ran into docker pull limits when pulling this image. Do we just assume that the buster image is hosted on a registry that is pullable in the env where this runs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@abhat, I am using the official go image from the docker registry, so most folks that have docker installed should be able to pull from the docker registry. Do you have any additional suggestions?

Per the snippet from the article at https://www.docker.com/blog/checking-your-current-docker-pull-rate-limits-and-status/ do you think this would be an issue?

Anonymous free users will be limited to 100 pulls per six hours, and authenticated free users will be limited to 200 pulls per six hours.

…tainer

Signed-off-by: Vishwanath Jayaraman <vjayaram@redhat.com>
@vjayaramrh vjayaramrh force-pushed the dockerize_tests branch 3 times, most recently from e36b396 to 2f9665d Compare January 27, 2021 22:14
Signed-off-by: Aniket Bhat <anbhat@redhat.com>
@trozet trozet changed the title allows for running all tests inside a golang:1.15.7-buster docker container allows for running all tests inside a container Jan 28, 2021
Copy link
Contributor

@trozet trozet left a comment

Choose a reason for hiding this comment

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

/lgtm

@trozet trozet merged commit a666cbc into ovn-org:master Jan 28, 2021
@vjayaramrh vjayaramrh deleted the dockerize_tests branch January 28, 2021 19:53
anfredette pushed a commit to anfredette/ovn-kubernetes that referenced this pull request Jan 29, 2021
Allows for running tests inside of a container using docker or podman.
 
Signed-off-by: Vishwanath Jayaraman <vjayaram@redhat.com>
Co-authored-by: Aniket Bhat <anbhat@redhat.com>
trozet pushed a commit to trozet/ovn-kubernetes that referenced this pull request Feb 22, 2021
Allows for running tests inside of a container using docker or podman.

Signed-off-by: Vishwanath Jayaraman <vjayaram@redhat.com>
Co-authored-by: Aniket Bhat <anbhat@redhat.com>
(cherry picked from commit a666cbc)
trozet pushed a commit to trozet/ovn-kubernetes that referenced this pull request Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants