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

Add GitHub Actions CI #147

Merged
merged 14 commits into from
Apr 3, 2020
Merged

Add GitHub Actions CI #147

merged 14 commits into from
Apr 3, 2020

Conversation

sethaxen
Copy link
Member

@sethaxen sethaxen commented Apr 2, 2020

Testing out using GitHub Actions as CI, as proposed in #144.

@sethaxen sethaxen added the WIP Work in Progress (for a pull request) label Apr 2, 2020
@codecov
Copy link

codecov bot commented Apr 2, 2020

Codecov Report

Merging #147 into master will decrease coverage by 3.86%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
- Coverage   97.68%   93.82%   -3.87%     
==========================================
  Files          48       48              
  Lines        3154     3012     -142     
==========================================
- Hits         3081     2826     -255     
- Misses         73      186     +113
Impacted Files Coverage Δ
src/groups/group.jl 75.13% <0%> (-22.25%) ⬇️
src/groups/translation_action.jl 81.25% <0%> (-18.75%) ⬇️
src/manifolds/Hyperbolic.jl 85.36% <0%> (-14.64%) ⬇️
src/manifolds/Grassmann.jl 88.88% <0%> (-11.12%) ⬇️
src/groups/metric.jl 84.61% <0%> (-11.04%) ⬇️
src/SizedAbstractArray.jl 80.48% <0%> (-9.99%) ⬇️
src/product_representations.jl 87.35% <0%> (-9.42%) ⬇️
src/manifolds/Circle.jl 90.83% <0%> (-9.17%) ⬇️
src/manifolds/Stiefel.jl 92.3% <0%> (-7.7%) ⬇️
src/manifolds/Euclidean.jl 91.35% <0%> (-7.48%) ⬇️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fa3d4f...7f1bc77. Read the comment docs.

@mateuszbaran
Copy link
Member

Do we need to trigger it on both push and pull_request?

@kellertuer
Copy link
Member

For documenter you might want to generate a set of keys – and do the GitHub Actions have a variable environment? You should then add the key as described here https://juliadocs.github.io/Documenter.jl/stable/man/hosting/#GitHub-Actions-1 – and since this is a different config file, a separate key should work, so we don't have to use the travis one twice. Or wa can anyways decide to do just one deploy (the one here?).

@sethaxen
Copy link
Member Author

sethaxen commented Apr 2, 2020

35 minutes to run the entire CI is pretty nice! Should be even lower with #146

Do we need to trigger it on both push and pull_request?

No, I'm going to remove push.

For documenter you might want to generate a set of keys – and do the GitHub Actions have a variable environment? You should then add the key as described here https://juliadocs.github.io/Documenter.jl/stable/man/hosting/#GitHub-Actions-1 – and since this is a different config file, a separate key should work, so we don't have to use the travis one twice. Or wa can anyways decide to do just one deploy (the one here?).

At the moment, everything seems to work for the docs build. It didn't deploy, but it's not yet configured to deploy the preview build on PRs. Yes, I think ultimately, we'll want to just deploy once. My thinking is that if we like GitHub Actions for CI, then we can remove Travis and Appveyor, since it works on all 3 OSes.

I've heard that codecov doesn't currently work if the branch isn't from the same repo. If we choose to merge this when completed, I'll open a PR from a fork to test this.

@sethaxen sethaxen removed the WIP Work in Progress (for a pull request) label Apr 3, 2020
@sethaxen
Copy link
Member Author

sethaxen commented Apr 3, 2020

Okay, this seems to me to be ready for review and possible merging (once tests pass). Perhaps we can run this along with Travis and Appveyor for a little while and see if we like it enough to switch entirely to it.

One thing I noticed that's pretty cool is that when Documenter runs, a "documenter/deploy".
job appears. Clicking "Details" next to that run opens up the preview docs URL!

@kellertuer
Copy link
Member

The preview URL is pretty cool, I always had to remember the URL style manually.

I think we can run them in parallel for a while, we just should do the deploy on only one, so remove it from travis maybe?

@sethaxen
Copy link
Member Author

sethaxen commented Apr 3, 2020

Good idea. Done.

@kellertuer
Copy link
Member

The [skip ci] does nor yet seem to work? Can you do that label-based approach?

@sethaxen
Copy link
Member Author

sethaxen commented Apr 3, 2020 via email

Copy link
Member

@kellertuer kellertuer 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 explanation, I wasn't aware of that magic part to avoid ci. Then this PR is fine with me.

@sethaxen sethaxen merged commit 1e4ca08 into master Apr 3, 2020
@sethaxen sethaxen deleted the github_actions branch April 3, 2020 17:15
@sethaxen
Copy link
Member Author

sethaxen commented Apr 3, 2020

The one thing to watch out for is Appveyor requires it in the title, while Travis reads title and description. Which I forgot for this PR, so when I merged it, the squashed commit description includes [skip ci], so Travis didn't run on the commit to master, while Appveyor did. The solution is editing the description before squashing to remove all [skip ci].

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