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

GHA Migration #103

Merged
merged 6 commits into from
Sep 28, 2022
Merged

GHA Migration #103

merged 6 commits into from
Sep 28, 2022

Conversation

claire-labry
Copy link
Contributor

@claire-labry claire-labry commented Sep 19, 2022

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.

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.

Copy link
Contributor

@samsalisbury samsalisbury left a comment

Choose a reason for hiding this comment

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

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.

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.

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

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.

@claire-labry claire-labry force-pushed the gha-migration branch 2 times, most recently from d801bc7 to f8ae56a Compare September 20, 2022 16:26
fi

# Install gotestsum with go get for 1.15.3; otherwise default to go install
- name: Install gotestsum
Copy link
Contributor Author

@claire-labry claire-labry Sep 20, 2022

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.

@claire-labry claire-labry marked this pull request as ready for review September 20, 2022 19:14
Copy link
Contributor

@samsalisbury samsalisbury left a comment

Choose a reason for hiding this comment

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

🚀

.github/workflows/go-tests.yml Outdated Show resolved Hide resolved
fi

# Install gotestsum with go get for 1.15.3; otherwise default to go install
- name: Install gotestsum
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.

Co-authored-by: Sam Salisbury <samsalisbury@gmail.com>
@claire-labry claire-labry merged commit b2e81e7 into main Sep 28, 2022
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