GHA Migration

[claire-labry]

This PR officially migrates go-version to GHA from CircleCI. It's intention is to work the same as the CircleCI tests but in GHA format.

Note: I've added a matrix that keeps go1.15.3 to ensure that we maintain compatibility and also added the latest go version [1.19] to ensure that we are maintaining the repo and keeping it up to date.

[mdeggies]

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 }}

[samsalisbury]

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?)

[claire-labry]

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.

[mdeggies]

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

[samsalisbury]

LGTM @claire-labry, just a couple of non-blocking notes inline that I think could be worth addressing just to make sure we're being intentional about any functional changes.

[samsalisbury]

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?

[claire-labry]

+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.

[samsalisbury]

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! :)

[claire-labry]

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.

[samsalisbury]

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.

[claire-labry]

@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?

[samsalisbury]

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.

[samsalisbury]

🚀

[samsalisbury]

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.