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

Adding github workflow to render docs and lint #848

Merged
merged 1 commit into from
May 14, 2021
Merged

Adding github workflow to render docs and lint #848

merged 1 commit into from
May 14, 2021

Conversation

vsoch
Copy link
Contributor

@vsoch vsoch commented Apr 30, 2021

This is almost all of the way there - the previous command to run tests does not work, and I suspect it's an issue with the path, or the install command (if anyone has insight that might take a look)!

In summary, this should be equivalent to the .travis-ci workflow, but run in GitHub actions. A few details:

-I am using the exact commit of the v1 release to install Go, as that is considered safer practice than the tag/release itself.

  • instead of the travis commit range, we can look for the environment variable for the GitHub sha and use that to derive a range instead.
  • Note that the gometalinter seems to be deprecated - but updating it to the new recommended one is probably out of scope for this PR.

Signed-off-by: vsoch [email protected]

@vsoch
Copy link
Contributor Author

vsoch commented Apr 30, 2021

Strange, I don't see the CI running here. Should I re-open a PR on my own branch and we can discuss over there?

@vsoch
Copy link
Contributor Author

vsoch commented Apr 30, 2021

The test command running locally (and in my repo CI) has this error - the one I'm suspecting means that I need to do some change to a path (e.g., GOPATH) or install differently:

$ go test $(go list ./... | grep -v /vendor/)
can't load package: package _/home/vanessa/Desktop/Code/image-spec/identity: cannot find package "_/home/vanessa/Desktop/Code/image-spec/identity" in any of:
	/usr/local/go/src/_/home/vanessa/Desktop/Code/image-spec/identity (from $GOROOT)
	/home/vanessa/Desktop/Code/go/src/_/home/vanessa/Desktop/Code/image-spec/identity (from $GOPATH)
	/home/vanessa/Desktop/Code/image-spec/src/_/home/vanessa/Desktop/Code/image-spec/identity
can't load package: package _/home/vanessa/Desktop/Code/image-spec/schema: cannot find package "_/home/vanessa/Desktop/Code/image-spec/schema" in any of:
	/usr/local/go/src/_/home/vanessa/Desktop/Code/image-spec/schema (from $GOROOT)
	/home/vanessa/Desktop/Code/go/src/_/home/vanessa/Desktop/Code/image-spec/schema (from $GOPATH)
	/home/vanessa/Desktop/Code/image-spec/src/_/home/vanessa/Desktop/Code/image-spec/schema
can't load package: package _/home/vanessa/Desktop/Code/image-spec/specs-go: cannot find package "_/home/vanessa/Desktop/Code/image-spec/specs-go" in any of:
	/usr/local/go/src/_/home/vanessa/Desktop/Code/image-spec/specs-go (from $GOROOT)
	/home/vanessa/Desktop/Code/go/src/_/home/vanessa/Desktop/Code/image-spec/specs-go (from $GOPATH)
	/home/vanessa/Desktop/Code/image-spec/src/_/home/vanessa/Desktop/Code/image-spec/specs-go
can't load package: package _/home/vanessa/Desktop/Code/image-spec/specs-go/v1: cannot find package "_/home/vanessa/Desktop/Code/image-spec/specs-go/v1" in any of:
	/usr/local/go/src/_/home/vanessa/Desktop/Code/image-spec/specs-go/v1 (from $GOROOT)
	/home/vanessa/Desktop/Code/go/src/_/home/vanessa/Desktop/Code/image-spec/specs-go/v1 (from $GOPATH)
	/home/vanessa/Desktop/Code/image-spec/src/_/home/vanessa/Desktop/Code/image-spec/specs-go/v1

That approach does seem to be suggested by internet wisdom: https://stackoverflow.com/questions/16353016/how-to-go-test-all-tests-in-my-project

@cyphar
Copy link
Member

cyphar commented May 1, 2021

There seems to be something funky going on with your GOPATH -- /usr/local/go/src/_/home/vanessa/Desktop/Code/ looks strange to me, but to be honest GOPATH issues always make my brain hurt.

Strange, I don't see the CI running here. Should I re-open a PR on my own branch and we can discuss over there?

I don't think you can create and run an action in a PR, the action needs to already exist on the main branch -- you might need to test it on your branch and then get us to go review it there.

@vsoch
Copy link
Contributor Author

vsoch commented May 1, 2021

Okay I finally have it working - I had to figure out the structure of the GitHub actions workspace, and then be a little more pendantic about paths. I wound up cloning the repository to where it should be in a gopath, a relative path at go/github.com/opencontainers/image-spec, and that is relative to the GITHUB_WORKSPACE which weirdly has the repository name twice, like /home/runner/work/image-spec/image-spec, which meant that the GOPATH was /home/runner/work/image-spec/image-spec/go.

I added the docs "output" folder to save as an artifact. It's not as good at CircleCI artifacts that you can preview, but it's better than nothing! Here are the successful checks: https://github.com/vsoch/image-spec/pull/2/checks.

There is detail work that could be done (e.g., more recent go versions, updating the latex container, adding better docs about building locally) but this should be a good skeleton to start.

@vbatts
Copy link
Member

vbatts commented May 12, 2021

oh nice! lemme review that now

Copy link
Member

@vbatts vbatts left a comment

Choose a reason for hiding this comment

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

LGTM

runs-on: ubuntu-latest
strategy:
matrix:
go: ['1.10', '1.11', '1.12']
Copy link
Member

Choose a reason for hiding this comment

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

these old versions are fine for first pass at parity. We can add the newer golang versions later

GOPATH: /home/runner/work/image-spec/image-spec/go
run: |
export PATH=$GOPATH/bin:$PATH
docker pull quay.io/oci/pandoc:1.17.0.3-2.fc25.x86_64
Copy link
Member

Choose a reason for hiding this comment

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

we'll have to remember to change this to the new image (https://github.com/opencontainers/distribution-spec/blob/main/Makefile#L11) once #846 merges

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep I saw that! If someone doesn't beat me to it, I'll follow #846 and can come back and PR to update the image in this workflow.

ifdef TRAVIS_COMMIT_RANGE
git-validation -q -run DCO,short-subject,dangling-whitespace
ifdef GITHUB_SHA
git-validation -q -run DCO,short-subject,dangling-whitespace -range $(GITHUB_SHA)..HEAD
Copy link
Member

Choose a reason for hiding this comment

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

ah, this ought to be tracked in git-validation.
vbatts/git-validation#48

path: go/src/github.com/opencontainers/image-spec

# commit for v1 release
- uses: actions/setup-go@0caeaed6fd66a828038c2da3c0f662a42862658f
Copy link
Member

Choose a reason for hiding this comment

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

wait, this isn't for the image-spec v1, but some github action's release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I am using the full commit because it’s safer than even a tag or release. https://github.community/t/are-github-actions-safe-to-use/17895/2

Copy link
Contributor

@jonboulle jonboulle left a comment

Choose a reason for hiding this comment

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

thanks for picking this up

.github/workflows/docs-and-linting.yml Outdated Show resolved Hide resolved
docker pull quay.io/oci/pandoc:1.17.0.3-2.fc25.x86_64
cd go/src/github.com/opencontainers/image-spec
make install.tools
go get -u github.com/alecthomas/gometalinter
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like this could just be moved into its own install.gometalinter target

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal was to reproduce the current CI, and making this call directly is exactly what it does:

- go get -u github.com/alecthomas/gometalinter

This should be equivalent to the .travis-ci workflow, but run
in GitHub actions. I am using the exact commit of the v1 release,
as that is considered safer practice than the tag/release itself.

Signed-off-by: vsoch <[email protected]>
Copy link
Member

@vbatts vbatts left a comment

Choose a reason for hiding this comment

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

LGTM

@jonjohnsonjr
Copy link
Contributor

jonjohnsonjr commented May 14, 2021

LGTM

Approved with PullApprove

@vbatts vbatts merged commit 32e130c into opencontainers:master May 14, 2021
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.

5 participants