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

Use GitHub actions #76

Merged
merged 3 commits into from
Jan 26, 2024
Merged

Use GitHub actions #76

merged 3 commits into from
Jan 26, 2024

Conversation

kiskoza
Copy link
Collaborator

@kiskoza kiskoza commented Jan 2, 2024

I was doing some maintanence tasks for an other gem and when I added the new Ruby 3.3 to the CI pipeline, I saw a few deprecation warnings coming from this gem (as one of our dependencies). I saw that there's a PR already to get rid of the warning (#75), but the project lacks of a working pipeline to verify that change (travis seems to be unavailable). Looking at the open pull requests, there were others trying to make sure that the gem works with recent Ruby versions (#74, #71).

Fast-forward to my PR: I'm trying to add Github Actions as it seems to be a reliable pipeline for open source repositories hosted on Github.

The changes I made:

  • replaced .travis.yml with .github/workflows/test.yml
  • dropped Ruby 1.8 from the CI: the ubuntu-20.04 image does not support installing it - I can look into if we can get it work on other base OS, but it's EOL for ages
  • dropped Ruby 1.9 from the CI: I wasn't able to setup the required gems, there's no compatible rexml version on rubygems: REXML gem specifies ruby > 0 but does not support < 2 ruby/rexml#38
  • included all 2.x and 3.x Ruby versions and ruby-head as allowed failure
  • included JRuby 9.3, 9.4 and jruby-head as allowed failure
  • added some Gemfile constrains for Ruby < 2.1 to make sure it passes the CI
  • added a Rakefile to make it easier to run the tests. we should convert the scripts folder to be rake tasks as well later
  • cherry-picked a commit from Add Ruby 3.1 support that is backwards compatible to Ruby 1.9.3 #74 to make the CI pass for recent Ruby versions

I hope you'll find this PR helpful. Let me know if you need anything else before you can merge it in.

(edit) And example of a green pipeline could be found here https://github.com/kiskoza/crack/actions/runs/7384963412

matrix:
allow_failures:
- rvm: 1.8
script: ./script/test
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why both introduce a new way to test on ci and not remove thr current way?

Otherwise. Looks good

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to clean this up - I wasn't sure if anybody still needs it to test the gem locally

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed script/test 👍

Copy link
Collaborator

@bf4 bf4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove out of scope changes

@@ -4,20 +4,32 @@
# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

require 'strscan'
require 'psych'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this change is in scope with ths pr

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I remember correctly, I had to add this to fix a failing test with one of the Ruby versions. I'm going to check it again and come back with the results

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without 644b0f7 / #74 , the CI fails for Ruby 3.1, 3.2, 3.3 and JRuby equivalents, a sample run could be found here: https://github.com/kiskoza/crack/actions/runs/7622992428/job/20762118748

How should we resolve this?

  1. Keep the commit fixing the code
  2. Keep the Ruby versions within the CI matrix, but merge them as failing
  3. Remove the newer Ruby versions and add them along the fix in a new PR

I would prefer 1 or 2 and wouldn't recommend doing 3, but happy to go the path you find the best

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think simplest is to merge this with passing ci (comment out or allow failures) then in fix pr add the failing matrix options

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you interested in being added as a maintainer? You seem committed and thoughtful, so...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the CI matrix to allow failures for recent Ruby versions, it should be fine now. I'd happily help as being a maintainer as well.

@bf4 bf4 merged commit 5944de6 into jnunemaker:master Jan 26, 2024
@bf4
Copy link
Collaborator

bf4 commented Jan 26, 2024

@kiskoza You've been granted the respect and responsibility of a maintainer by myself and @jnunemaker :) Thanks for your assistance!

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.

2 participants