projectblacklight / blacklight

Blacklight provides a discovery interface for any Solr (http://lucene.apache.org/solr) index.
http://projectblacklight.org/
Other
757 stars 257 forks source link

Allow in gemspec any 3.x view_component instead of stopping at 3.0.x #3053

Closed jrochkind closed 1 year ago

jrochkind commented 1 year ago

The gemspec previously allowed anything from 2.66.x up to 3.0.x -- but did not allow past 3.0.x. view_component is now up to 3.3.0,

The spec on view_component was expanded up from just 2.x greater than 2.66 in commit 1b7d6c7aa from @barmintor. This introduced locking to an upper bound of a view_component minor version, 3.0.x, when before we had only been locking to major version, any 2.x greater than 2.66. As view_component does semver, I suspect this was a mistake, and when opening up blacklight to 3.0.x, we meant to open it up to any future 3.x?

This does that.

jrochkind commented 1 year ago

I believe the failing tests are failing on main too -- I'm not sure because I can't get main to pass locally for me either, but it has even more failures for me locally, not sure why.

Failures on Rails 7.04, ruby 3.1, propshaft. (Other matrix elements aren't run, it quits after first failure):

rspec ./spec/components/blacklight/facet_item_pivot_component_spec.rb:42 # Blacklight::FacetItemPivotComponent has the facet hierarchy
rspec ./spec/components/blacklight/facet_item_pivot_component_spec.rb:36 # Blacklight::FacetItemPivotComponent links to the facet and shows the number of hits

Appreciate if anyone has any insight here. I don't believe these are related to my change, but I'm not sure what to do.

I wonder if anyone can confirm whether or not main is passing CI?

jrochkind commented 1 year ago

@jcoyne do you think this is mergeable now despite CI failures, do you know more than me?

barmintor commented 1 year ago

ViewComponent 3.0 was not yet released when this commit went in. Did you mean to reproduce the entirety of your PR request as a commit message?

jrochkind commented 1 year ago

Thanks @barmintor, good to know! So do you mean that you indeed did not intend to limit to 3.1.x instead of a more semver 3.x?

I tend to write descriptive commit messages, and then use a tool that copies the commit message to the PR, yes.

If you don't find the commit message helpful or appropriate, I'm happy to write it however you like!

If anyone has any idea what to do about test failures, please share. I believe they are outstanding on 'main' and I don't know what to do about them.

jrochkind commented 1 year ago

I'm not sure if it's intentional that only some of these jobs are marked "required"?

Right now, a non-required one is failing... and then things are configured to cancel other "required" jobs, so they don't pass, and we get required statuses not present. But maybe they are all intended to be "required", not sure?

I will try manually running the cancelled "required" jobs to see individually what jobs pass.

barmintor commented 1 year ago

@jrochkind to be honest, I don't find this commit message to be descriptive. It's long, but verbosity is not description: it repeats information that is in the revision history itself, asks a question that has an answer, and includes a speculation about the previous state that isn't founded.

Here's what I recommend as a descriptive commit message:

Relax view_component gemspec to permit any 3.x release
The pin to 3.0.x releases is no longer necessary now that 3.x is formally released.
jrochkind commented 1 year ago

OK, I guess different people have different attitudes toward commit message style -- what I left is what I'd want to see when doing "code archeology" to understand the context of the commit.

But I see you have different preferences and it's important to you here, it is not important to me so I'll go ahead and amend with your commit message!

jrochkind commented 1 year ago

(@barmintor I personally would not have used those words, because I still don't understand why a pin to 3.0.x was necessary or desirable before 3.0 was released! I could only write a message that had my understanding!)

barmintor commented 1 year ago

@jrochkind it was important because at the time ViewComponent 3 was an RC, and it was changing. We didn't know what the ViewComponent 3 API would actually be, and were making a conservative change to test against the release candidates in the CI matrix. This is reflected in the commit message of the commit you linked to: use ViewComponent 3 RC in one of the CI matrix runs

jrochkind commented 1 year ago

@barmintor Oh, I see, thanks! Limiting what versions you allowed forward, okay!

Of course, there was no way to know that 3.0.0 final would be compatible either! And it is allowed by < 3.1 -- I think that's what was confusing me. But that does limit range of potential damage at least, I see! I wonder if there would be a way to allow and test with 3.0.0 pre-releases without even allowing 3.0.0 final, only allowing pre-releases! But anyway, doesn't matter until it comes up again.

Anyone have any opinions on whether we can merge this if we verify carefully that failing tests are identical to failing tests on main, or whether we should just wait for main to be fixed? Either could seem reasonable to me. I personally probably won't have much more time available this week/month to try to fix main.

barmintor commented 1 year ago

@jrochkind thank you for updating the commit message. I think this change is good, but I would like to try to figure out why CI is failing on main before I merge new PRs.

barmintor commented 1 year ago

Rebased on main for CI

jrochkind commented 1 year ago

Thank you @barmintor!