-
Notifications
You must be signed in to change notification settings - Fork 309
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
First draft of GitHub Action #309
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! Comments inline.
.github/workflows/build.yml
Outdated
- name: Integration test | ||
run: make integration-test | ||
continue-on-error: true | ||
|
||
- name: Integration cleanup | ||
run: ./integration_tests/cleanup.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just drop the integration tests from CI. I'm pretty sure we broke them when we switched to Go modules. Did you have any luck getting them to run locally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I didn't successfully get this running locally. If you or someone else has a better understanding of how it should work, that might help with troubleshooting. I thought about reverting the repo to the last commit where TravisCI successfully passed and checking if the tests worked locally for me then, but I didn't go that far.
Since I did not get these tests working, I could remove my edits of the runner-base.Dockerfile file. I only modified it while trying to get it working and it's possible my edits, while minor, might move the file further from a functioning state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd leave the Dockerfile alone, and drop the integration tests from CI. Can you file a follow up issue to figure out:
- What they did
- How to run them now that Go modules exist?
@justinbastress might have insights, he wrote them originally, back in the dark days of Go development.
.github/workflows/build.yml
Outdated
- name: Install goimports | ||
run: sudo apt install golang-golang-x-tools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need goimports
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently goimports didn't need to be installed separately in TravisCI (based on the .travis.yml), but it does in GitHub Actions. It is used in the Makefile:
Line 21 in 5e9507c
goimports -w -l $(GO_FILES) |
I didn't try to understand why the Makefile was the way it was, so it's possible that the Makefile needs cleaning up as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's skip checking formatting in CI for now.
I minified the changes that this PR introduces and created a new issue around the tests I couldn't get working. Let me know if I missed anything. |
This does not run the integration tests. zmap#309
Aim to address #303 by migrating from TravisCI to GitHub Actions, because many issues with TravisCI have recently been reported.
How to Test
While in PR form, the GitHub Action build results will not be visible, since this repo is currently using only TravisCI. You will need to test this PR on your own branch and push the updated branch to GitHub to see the results in the Actions tab of GitHub.
After this action is part of the repo, it will be triggered by submitting a new PR or commit. The results will be seen in the Actions tab of this repo.
Notes & Caveats
Warning: This PR does not have full CI functionality in its initial form. The tests are currently encountering errors. I am no expert with Go build systems, so
continue-on-error: true
is used for the two test steps to get a passing result while we troubleshoot the errors encountered. I will probably need assistance from others to resolve these errors. If someone wants to borrow this code and create a new PR that works, go ahead. The sooner we have CI properly working again, the better.Issue Tracking
Issue #303