code4lib / ruby-oai

a Ruby library for building OAI-PMH clients and servers
MIT License
62 stars 42 forks source link

Move CI to github actions (fixes #87) #93

Closed barmintor closed 2 years ago

jrochkind commented 2 years ago

Nice thank you! I wonder why CI isn't actually running on this PR? Is it because @barmintor isn't a repo committer? But I don't see any buttons that would let me authorize the run here.

I feel like I'd like to see it succeed before merging it... but I can't figure out what to do there. Should I just merge it anyway?

@barmintor , would you like to be a ruby-oai repo committer/releaser? I would add you based on this work.

jrochkind commented 2 years ago

@barmintor actually setting up each Rails version in the ci.yml file, with a copy-paste-modify boilerplate... is not how I've seen this done before. Is this a way you have done it previously in other repos and liked, however?

I have seen it done instead using the github actions "matrix" feature -- possibly using multiple Gemfiles, possibly managed with the appraisal gem -- which gives you a more "DRY" workflow file.

Your way has the advantage of being simple and direct and easy to set up and that you've already written it, I guess. It has the disadvantage of making the ci.yml file kind of large and intimidating, and with many un-DRY parts that all need to be kept in sync if there are any updates, and it's a bit more difficult to add/remove Rails versions under test.

barmintor commented 2 years ago

I've aped the blacklight approach here. My recommendation is that you consider this a base from which you can refactor for legibility, but it does run the suite (you can see it in the actions tab on my fork).

barmintor commented 2 years ago

See also https://github.com/barmintor/ruby-oai/actions

barmintor commented 2 years ago

@jrochkind my principal concern is responding to your concerns over on Samvera, because this is a dependency of a blacklight plugin that I want to get shored up before the next major blacklight rev. If making me a committer here seems useful to you, I won't protest - but I will probably start doing some stuff in service of the aforementioned!

jrochkind commented 2 years ago

OK thanks. I think this can be very easily refactored to use github matrix for RAILS_VERSION, and I'd encourage considering to do the same on blacklight, I'm not sure why it's being done this way.

If I have commit writes to your branch, I may try committing the refactoring to it and seeing what happens? Not sure the best way for me to try to refactor is.

my principal concern is responding to your concerns over on Samvera,

I'm not sure what you mean about "my concerns over on Samvera", sorry! (Or did you mean to say Blacklight instead of Samvera, do you mean my recent concerns on that recent Blacklight PR? Still not sure how ruby-oai comes into that, so still not sure!)

If you are interested in being a maintainer here, I would be happy to add you, because there are a shortage of active maintainers. (I am not intending to be, just trying to keep the lights on). But it sounds like you may not be!

jrochkind commented 2 years ago

Oh wait, I got confused looking at Blacklight. The Blacklight ci.yml does not seem like the same approach has here to me?

Here you use different gemfiles you have checked into the repo; the blacklight one uses a RAILS_VERSION instead?

There are indeed a lot of different possible ways to do it. I guess I actually don't have time to figure out the best way to do this myself right now today after all, sorry.

barmintor commented 2 years ago

Right, I preserved the travis config as possible and otherwise tried to follow blacklight as a convention.

By "over at Samvera" I meant your comments in the Samvera slack a couple of weeks ago.

Since you are concerned (rightfully so, I think!) that there is no CI running right now, I've taken the path of least resistance. I recommend that you evaluate this on the basis of viability and not whether it is the best way to do it, since it can be refactored really at the whim of the committers (it shouldn't impact client applications in anyway).

barmintor commented 2 years ago

I had a look at using the appraisal gem (which is actually what generated the alternate gemfiles in the repo, as far as I can tell). I'm not sure that it would do much to tidy up the action definition: The rails versions aren't actually in a matrix against the ruby versions, so at least in my limited imagination of the day you end up with a very similar looking configuration.

barmintor commented 2 years ago

I was able to get a lot of the way there with adding some include and exclude data to the matrix, but removing the appraisal-generated gemfiles from the repo will be more work.

jrochkind commented 2 years ago

Thanks for the matrix, that's great!

Oh, we're already using appraisal? Cool, yeah, this is great! No need to remove appraisal, I like appraisal!

Thank you for this contribution!

jrochkind commented 2 years ago

@barmintor This isn't actually working though, when I look at the Actions output, all the runs are using Rails 7, the gemfile in the matrix is having no effect.

I actually have a free hour, I'll try to fix it in another PR right now.