-
Notifications
You must be signed in to change notification settings - Fork 255
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
migrate CI build from travis to GitHub Actions #507
migrate CI build from travis to GitHub Actions #507
Conversation
CI build is fail now. |
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.
Thanks for the PR! Some of the checks are passing, but there are some issues. See comments below.
.github/workflows/ci.yml
Outdated
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
ruby: [2.7, 3.0, 3.1, 3.2, head] |
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 these should be strings. The yaml parser is casting 3.0
to the integer 3
, and this causes setup-ruby to install the latest Ruby 3.x version (3.2) instead of the desired 3.0.x.
ruby: [2.7, 3.0, 3.1, 3.2, head] | |
ruby: ["2.7", "3.0", "3.1", "3.2", "head"] |
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 don't know that...
Thx, I'll fix it.
.github/workflows/ci.yml
Outdated
ruby-version: ${{ matrix.ruby }} | ||
bundler-cache: true | ||
- name: Run tests | ||
run: bundle exec rake test:units lint |
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.
We need to move lint
out into its own job. The version of Rubocop we are using does not work on newer versions of Ruby, so we can't have it run as part of the Ruby matrix.
run: bundle exec rake test:units lint | |
run: bundle exec rake test:units |
A separate job for Rubocop could look like this:
rubocop:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- name: Set up Ruby
uses: ruby/setup-ruby@v1
with:
ruby-version: "2.7"
bundler-cache: true
- name: Run rubocop
run: bundle exec rake lint
What do you think?
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.
IMO, It is better to update Rubocop version and settings propery, but It is not appropriate on this PR.
Another PR is proper.
Separating a test and a lint is sounds good for me, 1 task 1 purpose.
So I separate test and lint on this PR, thank you example one 🙏
Updating settings or so will try by future PR by someone.
README.md
Outdated
@@ -4,7 +4,7 @@ | |||
more servers. | |||
|
|||
[![Gem Version](https://badge.fury.io/rb/sshkit.svg)](https://rubygems.org/gems/sshkit) | |||
[![Build Status](https://travis-ci.org/capistrano/sshkit.svg?branch=master)](https://travis-ci.org/capistrano/sshkit) | |||
[![Build Status](https://github.com/capistrano/sshkit/actions/workflows/test.yml/badge.svg)](https://github.com/capistrano/sshkit/actions/workflows/test.yml) |
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 be ci.yml
?
[![Build Status](https://github.com/capistrano/sshkit/actions/workflows/test.yml/badge.svg)](https://github.com/capistrano/sshkit/actions/workflows/test.yml) | |
[![Build Status](https://github.com/capistrano/sshkit/actions/workflows/ci.yml/badge.svg)](https://github.com/capistrano/sshkit/actions/workflows/ci.yml) |
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.
oops I'll fix it.
47aad55
to
ccd93e4
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.
This looks great! One small remaining request about using the latest actions/checkout@v3
if you could take one more look. Thanks!
.github/workflows/ci.yml
Outdated
matrix: | ||
ruby: ["2.7", "3.0", "3.1", "3.2", "head"] | ||
steps: | ||
- uses: actions/checkout@v2 |
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 we should stay up to date with the latest version of checkout
, which is currently v3
. Do you mind changing it?
- uses: actions/checkout@v2 | |
- uses: actions/checkout@v3 |
.github/workflows/ci.yml
Outdated
rubocop: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v2 |
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.
Also here:
- uses: actions/checkout@v2 | |
- uses: actions/checkout@v3 |
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 reviewing the old Travis config and I am finding some more things that were overlooked. I'd like to maintain parity with what Travis used to provide for us.
.github/workflows/ci.yml
Outdated
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
ruby: ["2.7", "3.0", "3.1", "3.2", "head"] |
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 Travis build tested all the way back to Ruby 2.0. Backwards compatibility is a big deal for sshkit and so I wouldn't want to lose that coverage. Can you add those missing versions?
ruby: ["2.7", "3.0", "3.1", "3.2", "head"] | |
ruby: ["2.0", "2.1", "2.2", "2.3", "2.4", "2.5", "2.6", "2.7", "3.0", "3.1", "3.2", "head"] |
include: | ||
# Run Danger only once, on 2.5 | ||
- rvm: 2.5 | ||
script: bundle exec danger |
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.
Looks like danger
got left behind as well. Can you include running this as its own job in the GitHub actions config?
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.
okay I'll add it
ccd93e4
to
ded807e
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.
This is very close! One last request 🙏
.github/workflows/ci.yml
Outdated
- name: Run rubocop | ||
run: bundle exec rake lint | ||
|
||
danger: |
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.
Hmm. The danger
job is failing for some reason. Looks like setting it up will be more involved. Can you remove the danger
job for now? I can try adding it back in a separate PR.
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.
removing done 👍
d30227a
to
0ab3fa2
Compare
This PR migrate a CI build from Travis to GitHub Actions. closes: capistrano#504
0ab3fa2
to
2f05783
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.
Great! Thank you for the contribution! ✨
This PR migrate a CI build from Travis to GitHub Actions.
closes: #504