-
Notifications
You must be signed in to change notification settings - Fork 170
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
Transfer Circle CI Automation to Github Actions #335
Transfer Circle CI Automation to Github Actions #335
Conversation
So I reviewed and everything LGTM. But I wanted to chat about this a bit more:
So this caused a significant amount of pain for @sindhusegment and also for me afterwards needing to fix the release process when we lost the creds once and no one had any idea why. Basically an annoying problem is that the release is only ever executed on tag creation which maybe makes sense but also never tests a bunch of code. Do you think we should be testing this release flow during merge to master for example (not necessarily on every commit) so that we find out if it's broken prior to trying a release and discovering the problem? This is obviously out of scope for this ticket? Should we have another ticket to discuss the best way to solve this? Thoughts? |
go-version: ${{ matrix.go }} | ||
|
||
- name: Test | ||
run: make test |
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.
Do you know what the output of make test is? Do we know what the test coverage is and whether we're satisfied with it?
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 just looked at it in the job, but don't know the coverage, should we add a coverage report (backlog ticket)?
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.
That looks like it's ported over 1:1 from the circleci yaml; probably something we can reasonably say "this is fine" to and sleep well tonight.
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.
We've tried to port 1:1 without refactoring anything. We can improve chamber release separately.
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.
Overall LGTM, comments added about things but they're all out of scope potential backlog items to discuss later.
- name: Set up QEMU | ||
uses: docker/setup-qemu-action@v1 |
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'm assuming this is because docker is doing cross-compilation, and this is intentional (not copypasta?)
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.
Yes, it is because of that, we're building for multiple OS. We found this in this doc
@asaf-erlich We should think about a better process the test the release process without having to ship to master or on tag creation. Perhaps we can release a "pre-release" version when we merge to master or a even dryrun process for every commit in a feature branch? |
@asaf-erlich totally agree with you. This is something that we should definitely streamline and create a backlog ticket for |
LGTM. We might need to remove the circle webhook to get this to merge. The comments others had said are about what i would comment as well, so +1 to everything else but not blocking. nice work!~ |
@emmy-byrne-segment We will remove after we ensure actions is working. Given the unknowns about CircleCI permissions, I prefer to avoid having to reconfigure circleci webhook if necessary. |
Changes
TODO