Skip to content

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

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

Merged
merged 4 commits into from
Apr 19, 2022

Conversation

barmintor
Copy link
Collaborator

No description provided.

@jrochkind
Copy link
Collaborator

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
Copy link
Collaborator

jrochkind commented Apr 19, 2022

@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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

@barmintor
Copy link
Collaborator Author

@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
Copy link
Collaborator

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
Copy link
Collaborator

jrochkind commented Apr 19, 2022

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator

jrochkind commented Apr 19, 2022

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 jrochkind merged commit 12ef08b into code4lib:master Apr 19, 2022
@jrochkind
Copy link
Collaborator

@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.

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