Bug 24573 - Catmandu::Store::ElasticSearch and Catmandu::MARC are missing from cpanfile
Summary: Catmandu::Store::ElasticSearch and Catmandu::MARC are missing from cpanfile
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Julian Maurice
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 19735
Blocks:
  Show dependency treegraph
 
Reported: 2020-02-04 07:10 UTC by Julian Maurice
Modified: 2020-04-01 22:42 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 24573: Add missing dependencies to PerlDependencies.pm (1.24 KB, patch)
2020-02-04 07:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 24573: Add missing dependencies to cpanfile (900 bytes, patch)
2020-02-12 17:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 24573: Add missing dependencies to cpanfile (949 bytes, patch)
2020-03-02 20:17 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 24573: Add missing dependencies to cpanfile (963 bytes, patch)
2020-03-06 09:42 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 24573: Add missing dependencies to cpanfile (1.04 KB, patch)
2020-03-06 09:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2020-02-04 07:10:13 UTC

    
Comment 1 Julian Maurice 2020-02-04 07:10:59 UTC
Created attachment 98361 [details] [review]
Bug 24573: Add missing dependencies to PerlDependencies.pm

Catmandu::Store::ElasticSearch is required for search,
and Catmandu::MARC is required for indexation

The `max_ver` for Catmandu::Store::ElasticSearch is only informative and
is not used anywhere.
Koha is not compatible with Catmandu::Store::ElasticSearch 0.9_01 and
later versions
Comment 2 Julian Maurice 2020-02-12 17:07:25 UTC
Created attachment 98769 [details] [review]
Bug 24573: Add missing dependencies to cpanfile

Catmandu::Store::ElasticSearch is required for search,
and Catmandu::MARC is required for indexation
Comment 3 Michal Denar 2020-03-01 22:56:21 UTC
Hi Julian,
how test this patch?
Comment 4 Julian Maurice 2020-03-02 08:22:05 UTC
(In reply to Michal Denar from comment #3)
> Hi Julian,
> how test this patch?

Hi,

I think it's enough to `git grep Catmandu::MARC` and `git grep Catmandu::Store::ElasticSearch` to see that they are used in Koha (thus they must appear in cpanfile)

But you can go further and uninstall these modules, and see that search and reindexation don't work when SearchEngine = Elasticsearch

You can try the latest version of Catmandu::Store::ElasticSearch and verify that search doesn't work with it. Downgrade to 0.512 and verify that search works.
Comment 5 Michal Denar 2020-03-02 20:17:39 UTC
Created attachment 99959 [details] [review]
Bug 24573: Add missing dependencies to cpanfile

Catmandu::Store::ElasticSearch is required for search,
and Catmandu::MARC is required for indexation

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 6 Marcel de Rooy 2020-03-06 08:42:07 UTC
(In reply to Julian Maurice from comment #4)
> I think it's enough to `git grep Catmandu::MARC` and `git grep
> Catmandu::Store::ElasticSearch` to see that they are used in Koha (thus they
> must appear in cpanfile)

git grep Catmandu::MARC
cpanfile:requires 'Catmandu::MARC', '1.241';

Only used in the cpanfile ?
Comment 7 Marcel de Rooy 2020-03-06 08:48:13 UTC
> Catmandu::Store::ElasticSearch is required for search,
> and Catmandu::MARC is required for indexation

No, we still have Zebra. I think it should be a recommend then?
Comment 8 Julian Maurice 2020-03-06 08:56:35 UTC
(In reply to Marcel de Rooy from comment #6)
> (In reply to Julian Maurice from comment #4)
> > I think it's enough to `git grep Catmandu::MARC` and `git grep
> > Catmandu::Store::ElasticSearch` to see that they are used in Koha (thus they
> > must appear in cpanfile)
> 
> git grep Catmandu::MARC
> cpanfile:requires 'Catmandu::MARC', '1.241';
> 
> Only used in the cpanfile ?

We use Catmandu::Importer::MARC, which is part of Catmandu::MARC distribution
Comment 9 Julian Maurice 2020-03-06 09:00:14 UTC
(In reply to Marcel de Rooy from comment #7)
> > Catmandu::Store::ElasticSearch is required for search,
> > and Catmandu::MARC is required for indexation
> 
> No, we still have Zebra. I think it should be a recommend then?

There is no rule yet for deciding when a module should be required or recommended.
I think it should be required because when you turn on the feature, Koha will die if these modules are not installed, and there is nothing preventing you to turn on the feature.
Comment 10 Marcel de Rooy 2020-03-06 09:15:49 UTC
(In reply to Julian Maurice from comment #9)
> (In reply to Marcel de Rooy from comment #7)
> > > Catmandu::Store::ElasticSearch is required for search,
> > > and Catmandu::MARC is required for indexation
> > 
> > No, we still have Zebra. I think it should be a recommend then?
> 
> There is no rule yet for deciding when a module should be required or
> recommended.
> I think it should be required because when you turn on the feature, Koha
> will die if these modules are not installed, and there is nothing preventing
> you to turn on the feature.

It seems to be that they should not be required now, since we did not require them before in PerlDependencies. In one of the tests we are mocking the module for such a reason iirc.

In Discussion? Please mail to the dev list? Or dev meeting agenda point?
Comment 11 Julian Maurice 2020-03-06 09:39:43 UTC
(In reply to Marcel de Rooy from comment #10)
> It seems to be that they should not be required now, since we did not
> require them before in PerlDependencies. In one of the tests we are mocking
> the module for such a reason iirc.

They were totally missing from PerlDependencies.pm. I think it was more a mistake than a real willingness to make it optional.

> In Discussion? Please mail to the dev list? Or dev meeting agenda point?

If you think it's better to make it optional, that's fine by me.
The important part of the patch is the version requirements. I will change requires to recommends
Comment 12 Julian Maurice 2020-03-06 09:42:11 UTC
Created attachment 100226 [details] [review]
Bug 24573: Add missing dependencies to cpanfile

When using Elasticsearch, Catmandu::Store::ElasticSearch is required for
search, and Catmandu::MARC is required for indexation

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 13 Marcel de Rooy 2020-03-06 09:47:02 UTC
Created attachment 100227 [details] [review]
Bug 24573: Add missing dependencies to cpanfile

When using Elasticsearch, Catmandu::Store::ElasticSearch is required for
search, and Catmandu::MARC is required for indexation

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Martin Renvoize 2020-03-06 10:04:55 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 15 Joy Nelson 2020-04-01 22:42:07 UTC
not backported due to dependencies