-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Replace Circle CI with GitHub CI #3604
Conversation
|
@@ -1,6 +1,6 @@ | |||
name: Coveralls | |||
|
|||
on: ["push", "pull_request"] | |||
on: ["push"] |
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 line caused coveralls to run twice on each PR. Example: #3603
It should be enough to run it once
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.
If remove pull_request
options, PR(before merged) will not trigger CI.
run: yarn mobx test:flow | ||
|
||
- name: Test performance | ||
run: yarn mobx test:performance |
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.
Initially I wanted to have multiple dependant jobs (like build <- test, build <- test size) the same way it was implemented in CircleCI config. On CircleCI it was done using persist_to_workspace option:
Line 25 in 42f8ac0
- persist_to_workspace: |
There is an alternative way doing so using GitHub actions: https://docs.github.com/en/actions/using-workflows/storing-workflow-data-as-artifacts#passing-data-between-jobs-in-a-workflow
But each time I added actions/upload-artifact@v3
to the config the pipeline mysteriously disappeared from the check list and I saw only coveralls checks: b9b473d
So I decided to keep it flat and it worked.
The only thing that I didn't cover here is creating artifacts out of Can someone describe why was it needed? |
I would assume so you can investigate/compare the results. The test itself doesn't pose any limits, you have to check the results manually. |
@urugator Yes, it doesn't pose any limits. GitHub CI shows the whole output of the command so it looks like we don't need to create artifacts out of the performance result anymore: |
Well you could see the same output on CircleCI or not? So I guess that wasn't readable/convinient enough ... I dunno, @mweststrate will know better. Just for reference the report alone looks like this:
Is it about that |
@urugator It's not related to |
Oh wow, I never knew perf artifacts were saved, so I never looked at them :-). So I think the setup as is is fine with one job. The primary thing about perf tests is to make sure they don't break by a PR. I'm fine to just run those tests locally whenever we have a PR we fear might affect them. I'll disable the circle ci tests above as well to make sure they don't block anything anymore. |
@mweststrate Thanks, I see that Circle CI is no longer blocking the CI. May I merge this? |
Yes, LGTM! Thanks for picking up! |
There is a new
Build and check
check that includes all the checks from the Circle CI config file