fog / fog-libvirt

libvirt provider for fog
MIT License
16 stars 44 forks source link

Become Ruby 2.5+ and update Rubocop #89

Closed lzap closed 3 years ago

lzap commented 3 years ago

This repository contains .ruby-version set to 2.3.0 for some reason. This ancient Ruby does not even compile on modern distribution, I do not think it is practical to have this.

Let the developers to have their own Ruby versions and let's ensure code quality and compatibility via tests on CI. There is extensive test matrix and this patch extends it to 2.0 - 3.0.

Also let's update Rubocop so it works fine with more recent rules.

lzap commented 3 years ago

So 3.0 does not work, fog-xml does not compile. Therefore 2.0 - 2.7 it is.

There is one problem with Rubocop tho, it errors out with:

Error: RuboCop found unsupported Ruby version 2.0 in `required_ruby_version` parameter (in fog-libvirt.gemspec). 2.0-compatible analysis was dropped after version 0.50.
Supported versions: 2.4, 2.5, 2.6, 2.7, 3.0

I am not huge fan of this tool tho, if you guys agree I'd drop it for "rufo" - an opinionated Ruby formatter. Or just using "ruby -c" check for syntax checking.

ekohl commented 3 years ago

IMHO .ruby-version doesn't belong in a library. It make sense in an application which is targeting a specific Ruby version but libraries generally support multiple versions. :+1: to removal.

As for the minimum Ruby version, I'd consider going to Ruby 2.5+.

I do like Rubocop since it can point to actual bugs and is much more than just a formatter and syntax checker.

lzap commented 3 years ago

I am gonna ping the most active committers @plribeiro3000 and @voxik as well as @geemus

Once we agree on this one, I would like to add an integration test that would run against a real instance, of course, we would not run it on CI, but I would use it to test all PRs. I am not comfortable with merging anything without a real test. Then I will ask all authors to rebase and merge stuff in. Any comments are welcome!

lzap commented 3 years ago

As for the minimum Ruby version, I'd consider going to Ruby 2.5+.

I also think it is a very sane version to start with, fog-core still advertise 2.0 so plugins should probably stick with that:

https://github.com/fog/fog-core/blob/master/fog-core.gemspec#L21

I have opened a question if this could be reconsidered: https://github.com/fog/fog-core/issues/267

I am fine to keep Rubocop in, even enforce it on CI. But for that we either need to use an older version or really bump the minimum Ruby version to 2.4+.

Thanks for review, if folks are not against I would like to nominate @ekohl also for push permissions to have a peer for reviews to get things moving.

geemus commented 3 years ago

@lzap thanks for checking in.

I've been moving towards 2.5+ and also transitioning toward github actions over travis, you can see that config here: https://github.com/fog/.github/blob/main/workflow-templates/ruby.yml. I'd be happy to cut over from travis to actions for you if you'd like. I've done it with a number of other repos, so know exactly what needs done and can do it pretty quickly. We probably need to update some things (like the gemspec) to match these updated versions, I had just overlooked that previously. Does that work for you?

Dropping .ruby-version makes sense to me. I don't have strong feelings about rubocop vs other options, whatever works well for you all.

I've invited @ekohl to join us to help with reviews/etc.

lzap commented 3 years ago

Thanks a bunch.

So it looks like you landed on 2.5+ which is a reasonable compromise giving enough room to users who are not on the latest Ruby versions yet some extra time for migration. Core is making 2.5+ official so we can go ahead with this patch then: https://github.com/fog/fog/pull/4038

I am removing the IRC notification, the room is empty. I am keeping Rubocop then, let's just bump the version to the most recent one.

Let's wait a bit for other opinions and if we don't hear let's go ahead @ekohl ?

lzap commented 3 years ago

For the record, I am not enabling Rubocop yet on Travis, this will be done in a separate PR.

geemus commented 3 years ago

Sounds good.

geemus commented 3 years ago

I'll go ahead and add the github actions related stuff, it shouldn't hurt anything and can run in parallel. Then you can drop travis if/when you are ready. (I've usually done this pretty promptly once the actions seemed ready).

geemus commented 3 years ago

Ok, took a few tries but it seems to be working now. I'll note that 3.0 and head both seem to be failing when they try to install the ruby-libvirt gem. I'd welcome any advise on how to address that.

ekohl commented 3 years ago

Now that Github actions have been merged, should this instead be updated to drop Travis and then update Rubocop?

geemus commented 3 years ago

Works for me, although dropping travis could come later too.

I'll also note that I didn't have luck getting 3.0/head to work in actions, but they weren't being run in travis either, so I think it's still somewhat of an improvement.

Also realizing I forgot to update the build badge in the README, but can do that presently before I forget again.

ekohl commented 3 years ago

FYI: https://github.com/fog/fog-libvirt/pull/92 removes the RVM files and https://github.com/fog/fog-libvirt/pull/91 removes Travis. I can update the Travis removal with badge fixing.

ekohl commented 3 years ago

@geemus I'd appreciate it if you also used PRs for changes. This makes it easier to follow along for other maintainers. I also updated https://github.com/fog/fog-libvirt/pull/91 to fix the broken link in the status badge.

geemus commented 3 years ago

Thanks for being clear on your preferences. I'm happy to use PRs also, just in bad habits from doing a lot of this solo for a while.

lzap commented 3 years ago

Ok dropped the other changes, only updating Robocop, adding new GHA for it and updated minimum Ruby version in gemspec which slipped through.

lzap commented 3 years ago

Force-pushed.

What are we gonna do about Ruby 3.0 and trunk? Shall we disable them for now?

geemus commented 3 years ago

Yeah, I guess disabling makes sense? I've configured it to not stop the other runs on failure, but there isn't anyway to ignore them that I'm aware of (so it will always make things look red).