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

ci: migrate from CircleCI to GitHub Actions #605

Merged
merged 2 commits into from Aug 6, 2021
Merged

ci: migrate from CircleCI to GitHub Actions #605

merged 2 commits into from Aug 6, 2021

Conversation

Juneezee
Copy link
Contributor

@Juneezee Juneezee commented Aug 4, 2021

Completely migrates from CircleCI to GitHub Actions. Runs lint and test on every push and pull request.

@coveralls
Copy link

coveralls commented Aug 4, 2021

Pull Request Test Coverage Report for Build 545

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 57.369%

Totals Coverage Status
Change from base Build 535: 0.0%
Covered Lines: 3624
Relevant Lines: 6317

💛 - Coveralls

@Fontinalis Fontinalis requested a review from dhui August 4, 2021 08:15
Copy link
Member

@Fontinalis Fontinalis left a comment

Choose a reason for hiding this comment

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

I do like the idea of having everything in GitHub Actions and remove CircleCI from the list of tools we use and it's a great PR to replace tests on commits and PRs! 👍🏻

@dhui Thoughts?

Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and helping us improve our release process @Juneezee !

@Fontinalis

I do like the idea of having everything in GitHub Actions and remove CircleCI from the list of tools we use and it's a great PR to replace tests on commits and PRs! 👍🏻

@dhui Thoughts?

Agreed, we should move our tests to use GitHub Actions as well.
Let's keep the other CI and docker files for now in case we need to continue using them. We can clean them up later.

.github/workflows/ci.yaml Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

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

Add workflow_run to the GoReleaser workflow/job (.github/workflows/release.yaml) so that it only runs after tests pass

@Juneezee
Copy link
Contributor Author

Juneezee commented Aug 5, 2021

Add workflow_run to the GoReleaser workflow/job (.github/workflows/release.yaml) so that it only runs after tests pass

@dhui There is a caveat if we use workflow_run to trigger the goreleaser job, as described in this StackOverflow answer.

If the goreleaser job is triggered by workflow_run instead of on: push: tags: "v*", GoReleaser would not be able to detect tag that is pushed, which would then cause the job to fail.

EDIT: This leaves us to only 2 choices now (hoping that GitHub could improve the workflow dependencies in the future)

  1. Single workflow, dependent jobs: The only possible way at this moment to make goreleaser job run if and only if both lint and test jobs have passed.
  2. Separate workflow: This requires the maintainers to be careful and ensure that the tests passed on the latest commit before pushing a tag.

@Fontinalis
Copy link
Member

If the goreleaser job is trigerred by workflow_run instead of on: push: tags: "v*", GoReleaser would not be able to detect tag that is pushed, which would then cause the job to fail.

On the other hand, this could be solved if we merge the two workflows into one, and use the following code by @Juneezee to check if the release should run or not

  goreleaser:
    name: Release a new version
    environment: GoReleaser
    runs-on: ubuntu-latest
+   needs: [lint, test]
+   if: ${{ success() && github.repository == 'golang-migrate/migrate' && contains(github.ref, 'refs/tags/v') }}
    ...

So I'm happy to turn my vote on the question of "should we merge the two workflows or not".

@Juneezee Juneezee requested a review from dhui August 5, 2021 17:59
Fontinalis
Fontinalis previously approved these changes Aug 5, 2021
Copy link
Member

@Fontinalis Fontinalis left a comment

Choose a reason for hiding this comment

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

Looks legit to me! @dhui you concur?

Comment on lines 38 to 44
test-finish:
needs: [test]
runs-on: ubuntu-latest
steps:
- uses: shogo82148/actions-goveralls@v1
with:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed these lines in the previous commit. They are required for coveralls to work correctly with parallel tests, see https://github.com/shogo82148/actions-goveralls#parallel-job-example

@Juneezee
Copy link
Contributor Author

Juneezee commented Aug 5, 2021

Last round of review I think, everything seems to be well configured 🚀

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
At the time of this commit, it is not possible to make the `goreleaser`
job run in a separate workflow and detect the tag push event, see [1].

[1]: https://stackoverflow.com/a/68078768/7902371
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
@Juneezee Juneezee requested a review from dhui August 6, 2021 00:13
@Juneezee
Copy link
Contributor Author

Juneezee commented Aug 6, 2021

@dhui Please take a look again 😃

Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

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

Thanks for migrating our tests over!

@dhui dhui merged commit 5aa3b3a into golang-migrate:master Aug 6, 2021
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.

None yet

4 participants