Skip to content

Use golang image to run tests step#219

Merged
stefanprodan merged 1 commit into
fluxcd:mainfrom
relu:gha-golang-container
Nov 28, 2020
Merged

Use golang image to run tests step#219
stefanprodan merged 1 commit into
fluxcd:mainfrom
relu:gha-golang-container

Conversation

@relu
Copy link
Copy Markdown
Member

@relu relu commented Nov 26, 2020

No description provided.

@relu relu force-pushed the gha-golang-container branch 4 times, most recently from 8ba4670 to 4c6489c Compare November 26, 2020 15:40
@relu relu marked this pull request as ready for review November 26, 2020 15:42
@relu relu marked this pull request as draft November 26, 2020 15:42
@relu relu closed this Nov 26, 2020
@relu relu reopened this Nov 26, 2020
@relu relu force-pushed the gha-golang-container branch from 4c6489c to 4d327ac Compare November 26, 2020 15:48
Comment thread .github/workflows/e2e.yaml Outdated
env:
KUBEBUILDER_ASSETS: ${{ github.workspace }}/kubebuilder/bin
run: |
apk add --no-cache docker build-base git
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we need docker to run unit tests

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yup, copy pasta.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also Git is included in the go image

Comment thread .github/workflows/e2e.yaml
@relu relu force-pushed the gha-golang-container branch 7 times, most recently from f7c0010 to d7a3a47 Compare November 26, 2020 17:03
@relu
Copy link
Copy Markdown
Member Author

relu commented Nov 26, 2020

Seems we have a problem. The issue I stumbled upon earlier when testing with the single container seems to be reproducible. I'm gonna see if I can reproduce this locally with the alpine image as well.

EDIT: Seems this is caused by the fact that the container runs as root so tests that check for permissions will always fail because... root :)

Comment thread .github/workflows/e2e.yaml Outdated
--volume ~/go/pkg/mod:/root/go/pkg/mod \
--env KUBEBUILDER_ASSETS=$KUBEBUILDER_ASSETS \
--workdir $(pwd) golang:1.15-alpine /bin/sh -c \
"apk add --no-cache build-base git ; make test"
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I actually had to add git cause it was complaining that the git binary could not be located.

"apk add --no-cache build-base git ; make test"
env:
KUBEBUILDER_ASSETS: ${{ github.workspace }}/kubebuilder/bin
- name: Check if working tree is dirty
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is not necessary to run this in the container as we're mirror mounting the workdir into the container.

@relu relu force-pushed the gha-golang-container branch 5 times, most recently from 3b67516 to 4df1777 Compare November 26, 2020 18:29
@relu
Copy link
Copy Markdown
Member Author

relu commented Nov 26, 2020

Finally, a working version! 🎉

I resorted to creating a dedicated GitHub Action as the most elegant solution.

@relu relu force-pushed the gha-golang-container branch from 4df1777 to ee6b782 Compare November 26, 2020 18:43
@relu relu marked this pull request as ready for review November 26, 2020 18:43
@relu relu requested a review from stefanprodan November 26, 2020 18:43
@stefanprodan
Copy link
Copy Markdown
Member

You can remove the go mod cache action as it's useless now, an action container doesn't have access to the host go modules dir.

@stefanprodan
Copy link
Copy Markdown
Member

Ah it seems that you can mount host dirs https://stackoverflow.com/questions/58997638/docker-container-volume-does-not-get-mounted-in-github-actions

@relu
Copy link
Copy Markdown
Member Author

relu commented Nov 27, 2020

Yes, if you expand the build step in the log you'll see exactly which directories are mounted inside the container from the host. The home dir is one of them.

@stefanprodan
Copy link
Copy Markdown
Member

@relu if you look at the make test output for this PR, you'll see that it downloads all go modules, so as it is, the container doesn't have the go cache in the right place.

@relu
Copy link
Copy Markdown
Member Author

relu commented Nov 27, 2020

I'll look into it, I didn't realize it was not using the cache.

@relu relu force-pushed the gha-golang-container branch 6 times, most recently from 80d3833 to f5a722a Compare November 27, 2020 12:16
@relu
Copy link
Copy Markdown
Member Author

relu commented Nov 27, 2020

Ok, so I was wrong, it does not mount the actual home dir in the container but rather this location: /home/runner/work/_temp/_github_home. So we can instruct the cache to read/write from there instead of the regular home dir.

EDIT: Should be good now. I actually had to force set the GOPATH, I suspect the way the default value is set does not make use of the $HOME env var, but rather using a lower-level method (directly from /etc/passwd).

@relu relu force-pushed the gha-golang-container branch from f5a722a to 489d0a2 Compare November 27, 2020 12:30
@relu relu force-pushed the gha-golang-container branch from 489d0a2 to 27420c0 Compare November 27, 2020 13:01
FROM golang:1.15-alpine

# Add any build or testing essential system packages
RUN apk add --no-cache build-base git
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@phillebaba once this gets merged, you'll be able to add the libgit2-dev dep here for #213.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good!

Copy link
Copy Markdown
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @relu 🥇

@stefanprodan stefanprodan merged commit 1ea9999 into fluxcd:main Nov 28, 2020
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.

3 participants