CI: fix GHA CI not running unit tests in cmd #2972

kolyshkin · 2021-10-22T00:46:52Z

This fixes GHA CI not running unit tests in cmd, and simplifies some things along the way. For details, see commit messages.

Please review commit by commit.

The code was correct but too long. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

k8s-ci-robot · 2021-10-22T00:47:00Z

Hi @kolyshkin. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

bobbypage · 2021-10-22T00:48:03Z

/ok-to-test

kolyshkin · 2021-10-22T21:32:33Z

@bobbypage PTAL (this fixes quite a big issue in GHA CI)

kolyshkin · 2021-10-23T00:20:51Z

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Hmm, I followed the second link and got an impression that I need to be a kubernetes member in order to avoid waiting for /ok-to-test, and so I filed an issue about it (kubernetes/org#3053).

Only after doing that I realized that the first link points to https://github.com/orgs/google/people, not https://github.com/orgs/kubernetes/people.

@bobbypage please let me know if you're still OK to sponsor me for being a kubernetes member.

bobbypage · 2021-10-25T20:26:20Z

build/unit-in-container.sh

 }

 GO_FLAGS=${GO_FLAGS:-"-tags=netgo -race"}
-BUILD_PACKAGES=${BUILD_PACKAGES:-}
+PACKAGES=${PACKAGES:-"sudo"}


a little confused here, why do we need to install sudo?

It's in the commit message, copy/pasting here to save you a click:

Now, when these tests are enabled, we see that sudo is missing from the
image, so add it.

IOW some tests under ./cmd use sudo (although at the moment I don't remember which ones). Let me do this

-PACKAGES=${PACKAGES:-"sudo"} +PACKAGES=${PACKAGES:-"apt"}

in this PR and see what happens.

OK, I'm not sure why but tests passes. I remember sudo was needed for integration tests, and these are unit tests. Maybe I mixed those up when writing this. Will fix.

OK, there were 3 bugs in the code 🤦🏻

sudo was no needed;

s/BUILD_PACKAGES/PACKAGES/ was wrong, as tests need BUILD_PACKAGES to build test binaries;

GO_FLAGS were not propagated to docker container, thus tests were not performed with proper tags -- this was also the reason why (2) was not resulted in a ❌ CI.

All fixed now 😅

OK, apparently I mixed this up; sudo removed.

wow, thanks for the debugging and fixing this up :)

bobbypage · 2021-10-25T20:26:59Z

/ok-to-test

bobbypage · 2021-10-25T20:27:23Z

Thanks @kolyshkin for fixing this. +1 on your k8s sponsorship, happy to do that.

Makefile

kolyshkin · 2021-10-27T00:33:35Z

/hold

1. Honor GO_FLAGS; if unset, use -race. As a result, integration tests are now built with -race. 2. Instead of using -short to avoid integration tests, filter out packages under integration. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

So, build/unit-in-container.sh never ran any tests under cmd. This happened because the $(go list ...) in BUILD_CMD was not escaped, and therefore the expansion happened before cd. Here is a demo: [kir@kir-rhat cadvisor]$ BUILD_CMD="pwd && cd cmd && echo $(pwd)" [kir@kir-rhat cadvisor]$ echo $BUILD_CMD pwd && cd cmd && echo /home/kir/go/src/github.com/google/cadvisor The fix would be to escape the $ in $(go list): [kir@kir-rhat cadvisor]$ BUILD_CMD="pwd && cd cmd && echo \$(pwd)" [kir@kir-rhat cadvisor]$ echo $BUILD_CMD pwd && cd cmd && echo $(pwd) [kir@kir-rhat cadvisor]$ sh -c "$BUILD_CMD" /home/kir/go/src/github.com/google/cadvisor /home/kir/go/src/github.com/google/cadvisor/cmd Yet, it's easier just to reuse the make test target which already has the exclusion logic. For that to work properly with custom test configs from ./build/config, we need to export GO_FLAGS env var into container. While at it, slightly simplify the BUILD_CMD by adding -e to bash, so we do not have to specify && every time. Fixes: 74b513b Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

1. Go package no longer needs to be in a particular directory (since Go 1.10 or so). 2. fetch-depth: 1 is the default -- no need to specify it. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

There is no need to spawn a docker container for unit tests; run those locally. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2021-10-27T16:01:30Z

no longer a draft; PTAL @bobbypage

bobbypage · 2021-10-27T23:18:40Z

LGTM, thanks again for fixing this up.

build/unit-in-container.sh: simplify runtime search

b36502e

The code was correct but too long. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

google-cla bot added the cla: yes label Oct 22, 2021

k8s-ci-robot added the needs-ok-to-test label Oct 22, 2021

kolyshkin mentioned this pull request Oct 22, 2021

cgroup cleanups #2959

Merged

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Oct 22, 2021

bobbypage reviewed Oct 25, 2021

View reviewed changes

Makefile Show resolved Hide resolved

kolyshkin marked this pull request as draft October 27, 2021 00:33

kolyshkin force-pushed the fix-ci branch 2 times, most recently from 5491ea6 to 76b5d28 Compare October 27, 2021 01:31

kolyshkin added 2 commits October 26, 2021 18:38

Makefile: simplify and fix test targets

0600934

1. Honor GO_FLAGS; if unset, use -race. As a result, integration tests are now built with -race. 2. Instead of using -short to avoid integration tests, filter out packages under integration. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin force-pushed the fix-ci branch 4 times, most recently from 7beb677 to 2cdb020 Compare October 27, 2021 02:16

ci: simplify test-integration yaml

de4c69a

1. Go package no longer needs to be in a particular directory (since Go 1.10 or so). 2. fetch-depth: 1 is the default -- no need to specify it. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin force-pushed the fix-ci branch 4 times, most recently from 6a08747 to 2ae3813 Compare October 27, 2021 02:32

ci: run unit tests locally; simplify config

4421e25

There is no need to spawn a docker container for unit tests; run those locally. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin force-pushed the fix-ci branch from 2ae3813 to 4421e25 Compare October 27, 2021 02:35

kolyshkin marked this pull request as ready for review October 27, 2021 16:01

kolyshkin requested a review from bobbypage October 27, 2021 16:01

bobbypage approved these changes Oct 27, 2021

View reviewed changes

bobbypage merged commit 2ca0f4f into google:master Oct 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: fix GHA CI not running unit tests in cmd #2972

CI: fix GHA CI not running unit tests in cmd #2972

kolyshkin commented Oct 22, 2021

k8s-ci-robot commented Oct 22, 2021

bobbypage commented Oct 22, 2021

kolyshkin commented Oct 22, 2021

kolyshkin commented Oct 23, 2021

bobbypage Oct 25, 2021

kolyshkin Oct 26, 2021

kolyshkin Oct 26, 2021

kolyshkin Oct 27, 2021

kolyshkin Oct 27, 2021

bobbypage Oct 27, 2021

bobbypage commented Oct 25, 2021

bobbypage commented Oct 25, 2021

kolyshkin commented Oct 27, 2021

kolyshkin commented Oct 27, 2021

bobbypage commented Oct 27, 2021

CI: fix GHA CI not running unit tests in cmd #2972

CI: fix GHA CI not running unit tests in cmd #2972

Conversation

kolyshkin commented Oct 22, 2021

k8s-ci-robot commented Oct 22, 2021

bobbypage commented Oct 22, 2021

kolyshkin commented Oct 22, 2021

kolyshkin commented Oct 23, 2021

bobbypage Oct 25, 2021

Choose a reason for hiding this comment

kolyshkin Oct 26, 2021

Choose a reason for hiding this comment

kolyshkin Oct 26, 2021

Choose a reason for hiding this comment

kolyshkin Oct 27, 2021

Choose a reason for hiding this comment

kolyshkin Oct 27, 2021

Choose a reason for hiding this comment

bobbypage Oct 27, 2021

Choose a reason for hiding this comment

bobbypage commented Oct 25, 2021

bobbypage commented Oct 25, 2021

kolyshkin commented Oct 27, 2021

kolyshkin commented Oct 27, 2021

bobbypage commented Oct 27, 2021