-
Notifications
You must be signed in to change notification settings - Fork 16.5k
[Build] Add Github workflows #9490
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
Conversation
|
See the builds in action: ktmud#25 |
dpgaspar
left a comment
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.
First pass
.github/workflows/superset.yml
Outdated
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.
Should this include superset-frontend/** ?
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.
This file is purely for python code. Frontend code checks are done in other workflows. Maybe I this rename this file to avoid confusion.
|
Looks good - have you thought about Circle? |
|
CircleCI was my first choice, but it seems there are some administration overheads involved in setting it up and no other Apache projects are using it AFAIK. |
Codecov Report
@@ Coverage Diff @@
## master #9490 +/- ##
=======================================
Coverage 58.77% 58.77%
=======================================
Files 385 385
Lines 12239 12239
Branches 3022 3022
=======================================
Hits 7193 7193
Misses 4862 4862
Partials 184 184 Continue to review full report at Codecov.
|
04fd288 to
fd5c4ec
Compare
ktmud
left a comment
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 this is ready for another review. Everything finally works!
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.
The parallel mode is not currently in use.
.github/workflows/superset-e2e.yml
Outdated
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.
GitHub-hosted action runners have some softwares pre-installed, including MySQL, Redis, and PostgreSQL.
Here we map all database services to a new port to make sure we don't accidentally use the build-in services that may be running on default ports already.
.github/workflows/superset-e2e.yml
Outdated
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.
Cypress plugin will handle its own cache. So not need to add cypress-base/package-lock.json as hash key.
.github/workflows/superset-e2e.yml
Outdated
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 enabling recording as it's free anyway. Check the recorded logs here.
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.
Local builds may generate this folder.
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.
The original config has duplicate requestTimeout.
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.
Updated to a project under my personal account for testing. Will need to change back to the official project once merged.
Someone with admin access on GitHub please add a new record key to GitHub Actions secrets under the name CYPRESS_RECORD_KEY.
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.
The upgraded Cypress will error out because it was dragged to a zero-height element. This fixes the error by dragging to the last visible row instead of the invisible one.
ktmud
left a comment
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.
Updated the Cypress test cases even more.
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 need to actually wait for 20s for default commands (e.g., click triggers).
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.
A simple get will wait for the element to appear. No need to wait for the additional request.
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.
Has to return a Promise for the test cases to be captured.
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.
Again, a simple get will wait. Also, dashboardId may be out of sync when test cases are running in parallelized mode.
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.
The error message changed in Cypress v4.
d25aed5 to
5114064
Compare
288e466 to
bf3288e
Compare
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.
explore/visualizations/index.test.js was probably unintentioally disabled by #6241 .
It has since stopped running for over a year now. A lot of these tests are now broken. It's easy to add them back, but we may want to do the fix in another PR. Keep the changes for BigNumber here just for reference.
As an replacement for Travis CI.
|
Woking on further improvements, closing for now to avoid excessive notification. |
|
Hey @ktmud - let us know if there's anything we at Preset can do to support you on this work. We're really feeling the Travis pain and want to help get to a solution. |
CATEGORY
SUMMARY
Adding GitHub Actions per discussion in #9481. It is a pure addition to current build process for now. Nothing changes for Travis CI, except Cypress is upgraded from
3.6.1to4.3.0.We may evaluate whether to disable Travis CI once GitHub Actions become more stable.
Jobs are split into 3 workflows to maximize parallelization:
A custom GitHub Action was created to generalize some of the building steps, but not all of them. There are duplicate code about cache across files, while could potentially be lifted into the GitHub Action like the official Cypress GitHub action.
Some thoughts
GitHub Actions is less powerful than some other CI/CD tools, e.g., CircleCI and GitLab CI/CD, but overall it was a pleasant experience to use. Two major nuances are: 1. limited parallelization support, cannot organize jobs as DAGs; 2. cannot automatically cancel current jobs on new pushes.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
Make sure CI check pass.
ADDITIONAL INFORMATION
REVIEWERS
@john-bodley @dpgaspar @mistercrunch @craig-rueda