-
Notifications
You must be signed in to change notification settings - Fork 70
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
Switch to Github Actions #399
Conversation
0a05cf6
to
03fd080
Compare
03fd080
to
440bba3
Compare
|
||
jobs: | ||
audit: | ||
runs-on: ubuntu-latest |
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.
What are the options here? Is this the platform that we encourage people to use?
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.
Virtual environment | YAML workflow label |
---|---|
Windows Server 2019 | windows-latest or windows-2019 |
Windows Server 2016 R2 | windows-2016 |
Ubuntu 18.04 | ubuntu-latest or ubuntu-18.04 |
Ubuntu 16.04 | ubuntu-16.04 |
macOS X Mojave 10.14 | macOS-latest or macOS-10.14 |
Not necessarily, but it's what we were building on Circle already. If we want to support others, we can add steps to build it on other platforms too (as a separate PR)
@@ -0,0 +1,22 @@ | |||
name: Build and Test |
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.
Is there a semantic reason why the name is at the top for this one?
440bba3
to
b0c03b7
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.
If we end up going with this let's not forget to remove the Circle job which is currently failing with a not configured msg.
Also it looks like the build & test jobs are indeed taking much more time: https://github.com/interledger-rs/interledger-rs/pull/399/checks?check_run_id=258562730
@@ -0,0 +1,27 @@ | |||
on: | |||
pull_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.
Shouldn't cargo audit
be run on: push
, instead of on: pull_request
?
steps: | ||
- uses: actions/checkout@v1 | ||
- id: component | ||
uses: actions-rs/components-nightly@v1 |
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 we be using nightly here? Just being conservative in case nightly clippy tells us to do something that works in nightly, but does not work on stable
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.
Can't we use SSH for the host with GitHub actions? I think that is a huge disadvantage. We would have difficulties in investigating some errors in the CI process itself.
So personally I'm not so in favor of GitHub actions so far though the test results look very well-integrated, that is awesome.
Since this is quite a bit slower than Circle, I think we should hold off on this switch for now. We can revisit this later when they add caching |
This switches the project to use Github actions instead of CircleCI and is an alternative to #398.
Advantages of Github Actions:
Advantages of Circle:
Resolves #292
What do you think?