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.

Sign up for GitHub

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

Jump to bottom

GHA Migration #103

Merged
merged 6 commits into from
Sep 28, 2022
Merged

GHA Migration #103

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3666f5c
remove circle config directory
claire-labry Sep 19, 2022
f389813
migrate to gha
claire-labry Sep 19, 2022
10a0ac4
PR feedback
claire-labry Sep 20, 2022
e033ba8
fix install gotestsum for 1.15.3
claire-labry Sep 20, 2022
daeb99e
whoops forgot to wrap statement
claire-labry Sep 20, 2022
07809e1
Update .github/workflows/go-tests.yml
claire-labry Sep 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 0 additions & 60 deletions .circleci/config.yml
View file
Open in desktop

This file was deleted.

66 changes: 66 additions & 0 deletions .github/workflows/go-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
name: go-tests

on: [push]

env:
TEST_RESULTS: /tmp/test-results

jobs:

go-tests:
runs-on: ubuntu-latest
strategy:
matrix:
go-version: [1.19]
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a library that previously targeted go1.15.3, I wonder if we should add that version to the matrix as well to ensure we're still maintaining compatibility? If we don't want that, maybe we should explicitly call out this update in the PR description for the benefit of people looking back in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 -- excellent point, I'll add 1.15.3 into the matrix and also add a note that it was left there just in case.


steps:
- name: Setup go
uses: actions/setup-go@v3
with:
go-version: ${{ matrix.go-version }}

- name: Checkout code
uses: actions/checkout@v3

- name: Create test directory
run: |
mkdir -p ${{env.TEST_RESULTS}}

- name: Download go modules
run: go mod download

- name: Cache / restore go modules
uses: actions/cache@v3
with:
path: |
~/.cache/go-build
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously we only cached ~/go/pkg/mod just wondering why we're also caching the go build cache as well in this change? I can see the logic in wanting to, but I'm wondering if it's useful to do this unless we also update the key to take into account the go version? Also, since Go doesn't automatically trim the build cache, it's likely to keep growing larger over time due to the restore-keys being set to always restore something even without an exact cache hit based on go.sum. For simplicity I would favour not caching this directory, unless it has a significant impact on build time, in which case we should cache it more conservatively, separate from the mod cache, based only on exact cache hits based on go.sum and the go version. Hope that makes sense, happy to talk through any of this if not! :)

Copy link
Contributor Author

@claire-labry claire-labry Sep 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ah this makes total sense -- I was actually following the format that was suggested in the cache action repo here +1 to removing for now as it sounds like it would just bog things down, but can definitely revisit this in the future if its needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, interesting, thanks for the link to the docs... In the short term for a big project it might make things faster, but I doubt the impact will be very significant for this repo even in the short term when the cache size is small.

~/go/pkg/mod
key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}
restore-keys: |
${{ runner.os }}-go-

# Check go fmt output because it does not report non-zero when there are fmt changes
- name: Run gofmt
run: |
go fmt ./...
files=$(go fmt ./...)
if [ -n "$files" ]; then
echo "The following file(s) do not conform to go fmt:"
echo "$files"
exit 1
fi

- name: Install gotestsum
Copy link
Contributor Author

@claire-labry claire-labry Sep 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@samsalisbury could I get a re-review on the install gotestsum job? The job for 1.15.3 had failed when I pushed up the feedback. I had to wrap an if/else statement as 1.15.3 requires go get to install gotestsum but newer Go versions use the go install convention. The checks show that everything is being downloaded properly & passing but wanted to make sure the logic here makes sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

The logic looks good to me. I made a suggestion, but really do make your own mind up on whether you like it! I think your version might be more popular :) Thanks.

run: go install gotest.tools/[email protected]

- name: Run go tests
run: |
PACKAGE_NAMES=$(go list ./...)
gotestsum --format=short-verbose --junitfile $TEST_RESULTS/gotestsum-report.xml -- $PACKAGE_NAMES

# Save coverage report parts
- name: Upload and save artifacts
uses: actions/upload-artifact@v3
with:
name: Test Results
path: ${{env.TEST_RESULTS}}
Copy link
Member

Choose a reason for hiding this comment

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

This looks really good! Great job, Claire.

One tiny stylistic ❓ , we typically add spaces between brackets where we evaluate env vars. Is that something we want to be consistent about in this file? E.g. ${{env.TEST_RESULTS}} => ${{ env.TEST_RESULTS }}

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a plugin for VSCode that automates doing this @mdeggies? I wonder if we should advise everyone to use it if so, so this kind of thing doesn't come up as often? (Or are you using a different editor @claire-labry?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding spacing makes sense to me as it reads cleaner! I'll get that fixed in. @sam I use VSCode, yeah -- I can definitely look up if there's a plugin that'll automatically cleanup/space out between brackets, etc.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I'm not too helpful here, I just do this manually and try to stay consistent in the file I'm working in.