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 to GitHub Actions from CircleCI, allow running Browserstack on forked repo via label #2417

Merged
merged 17 commits into from
Aug 17, 2020

Conversation

ylemkimon
Copy link
Member

@ylemkimon ylemkimon commented Aug 12, 2020

From #2415:

Pros:

  • Nice integration with GitHub
  • Single codebase/infrastructure
  • Able to test on MacOS and Windows
  • Easier to view results
  • Annotation support
  • Doesn't have problem with forked repo enabling CI

Cons:

  • Not possible to reuse steps. The amount of code duplication is a bit high (Next Steps for Fully Functioning Composite Actions actions/runner#646)
  • Harder to view artifacts, e.g., screenshot diffs and new screenshots. They are zipped and have to be downloaded
  • Cannot skip if code is not changed, with required status checks enabled (but they only take <2 min. and GitHub Actions hour is unlimited)
  • [ci skip] or [skip ci] doesn't work for pull requests
  • Disable CircleCI
  • Disable Screenshotter bot
  • Remove Screenshotter bot GitHub App
  • Remove CircleCI from required status checks
  • Add GitHub Actions to required status checks
  • Update other pull requests

Copy link
Member

@kevinbarabash kevinbarabash left a comment

Choose a reason for hiding this comment

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

That was fast. Thanks for moving us over to GitHub Actions.

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.circleci/config.yml Show resolved Hide resolved
@kevinbarabash
Copy link
Member

Why did the coverage report change so drastically? I wouldn't expect any change in those numbers from this PR.

@ylemkimon

This comment has been minimized.

@KaTeX KaTeX deleted a comment from codecov-commenter Aug 13, 2020
@ylemkimon
Copy link
Member Author

ylemkimon commented Aug 13, 2020

It seems the codecov-action is uploading coverage/coverage-final.json, which was not uploaded with codecov-node. This file seems to have different method of counting lines, so coverage seems to have changed.

@ylemkimon ylemkimon changed the title ci: migrate to GitHub Actions from CircleCI ci: migrate to GitHub Actions from CircleCI, allow running Browserstack on forked repo via label Aug 14, 2020
@kevinbarabash
Copy link
Member

@ylemkimon do you need my help with any of the checkbox items at the top?

@ylemkimon
Copy link
Member Author

@kevinbarabash No, they are to-dos after this PR is merged.

@kevinbarabash
Copy link
Member

In that case I better review this. 🙂

Copy link
Member

@kevinbarabash kevinbarabash 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 CircleCI stuff to GitHub Actions. It'll be nice to have all of the CI workflows using the same system. Was #2418 a test for this PR? If so, wouldn't the base of that PR need to be the head of this one? I think it would be good to also do a PR from a fork to test that flow. Also, more comments especially for some of the if statements in the ci.yml will be helpful if other people need to look at the file in the future.

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved

steps:
- id: set-matrix
uses: actions/github-script@v2
Copy link
Member

Choose a reason for hiding this comment

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

This docs for this action say that it's possible to require external scripts. If we did that we could lint the script.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kevinbarabash The repo has to be checked out in order to run the script on the repo. However, this job doesn't check out the repo, and adding checkout will increase the time by about 5 seconds, where this job takes only 1-2 seconds.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize that. That's good to know.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kevinbarabash I'm creating a ESLint plugin, which lints JavaScript in GitHub Actions workflow. I'll enable it later.

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
dockers/screenshotter/screenshotter.js Outdated Show resolved Hide resolved
dockers/screenshotter/screenshotter.js Outdated Show resolved Hide resolved
Copy link
Member

@kevinbarabash kevinbarabash left a comment

Choose a reason for hiding this comment

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

LGTM. There are a couple of more places in ci.yml that could use comments before merging this, see inline comments for details.

@ylemkimon ylemkimon merged commit 221042b into master Aug 17, 2020
@ylemkimon
Copy link
Member Author

@kevinbarabash Thank you for the review! I've confirmed they work as intended in #2429. Could you stop CircleCI builds in https://app.circleci.com/settings/project/github/KaTeX/KaTeX?

@kevinbarabash
Copy link
Member

I've stopped the CircleCI builds.

@edemaine
Copy link
Member

Exciting! Nice work.

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