Bug 19462

Summary: Add a koha-elasticsearch command
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Command-line UtilitiesAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Nick Clemens <nick>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, mtompset, robin
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19465
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
A new command-line script is added for handling Elasticsearch indexing-related tasks for each Koha instance.
Version(s) released in:
Attachments: Bug 19462: Add a koha-elasticsearch command
Bug 19462: Add koha-elasticsearch documentation
Bug 19462: Add a koha-elasticsearch command
Bug 19462: Add koha-elasticsearch documentation
Bug 19462: Add a koha-elasticsearch command
Bug 19462: Add koha-elasticsearch documentation

Description Tomás Cohen Arazi 2017-10-13 15:29:51 UTC
There should be a wrapper command for instances, with (at least) the ability to trigger index rebuilds on Elasticsearch.
Comment 1 Tomás Cohen Arazi 2017-10-13 18:17:45 UTC
Created attachment 68091 [details] [review]
Bug 19462: Add a koha-elasticsearch command

This patch adds a new command to be used for Elasticsearch-related
tasks.

The current implementation only offers the --rebuild action switch,
that allows reindexing Elasticsearch on a per-instance basis as we are
used to with the rest of the koha-* commands.

Other options could be added in a future: --status (ES server status
report, etc).

To test:
- Apply the whole patchset
- Have a suitable Koha+Elasticsearch setup [1]
- Run:
  $ reset_all
- Run:
  $ man koha-elasticsearch
=> SUCCESS: A pretty man page is displayed covering all options
- Run:
  $ sudo koha-elasticsearch blah
=> SUCCESS: The script fails because blah is not a valid instance name
- Run:
  $ sudo koha-elasticsearch -v kohadev
=> SUCCESS: Reindex happens!
- Try the -a, -b and -c option switches
- Sign off happily :-D

Sponsored-by: ByWater Solutions

[1] This is straightforward if you are using KohaDevBox and created the
box using: $ KOHA_ELASTICSEARCH=1 vagrant up

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2017-10-13 18:17:51 UTC
Created attachment 68092 [details] [review]
Bug 19462: Add koha-elasticsearch documentation

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Mark Tompsett 2017-10-14 03:20:30 UTC
Created attachment 68118 [details] [review]
Bug 19462: Add a koha-elasticsearch command

This patch adds a new command to be used for Elasticsearch-related
tasks.

The current implementation only offers the --rebuild action switch,
that allows reindexing Elasticsearch on a per-instance basis as we are
used to with the rest of the koha-* commands.

Other options could be added in a future: --status (ES server status
report, etc).

To test:
- Apply the whole patchset
- Have a suitable Koha+Elasticsearch setup [1]
- Run:
  $ reset_all
- Run:
  $ man koha-elasticsearch
=> SUCCESS: A pretty man page is displayed covering all options
- Run:
  $ sudo koha-elasticsearch blah
=> SUCCESS: The script fails because blah is not a valid instance name
- Run:
  $ sudo koha-elasticsearch -v kohadev
=> SUCCESS: Reindex happens!
- Try the -a, -b and -c option switches
- Sign off happily :-D

Sponsored-by: ByWater Solutions

[1] This is straightforward if you are using KohaDevBox and created the
box using: $ KOHA_ELASTICSEARCH=1 vagrant up

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 4 Mark Tompsett 2017-10-14 03:20:33 UTC
Created attachment 68119 [details] [review]
Bug 19462: Add koha-elasticsearch documentation

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 5 Mark Tompsett 2017-10-14 03:24:51 UTC
After getting ES to work, and adjusting my expectations of searches, this runs minimally well. It could be polished on edge error cases, but nothing that merits a Failed QA in my opinion.

For example, try a "-c=2" instead of a "-c 2". Ugliness ensues.
Comment 6 Tomás Cohen Arazi 2017-10-25 15:02:38 UTC
(In reply to M. Tompsett from comment #5)
> After getting ES to work, and adjusting my expectations of searches, this
> runs minimally well. It could be polished on edge error cases, but nothing
> that merits a Failed QA in my opinion.
> 
> For example, try a "-c=2" instead of a "-c 2". Ugliness ensues.

Are we supposed to support -c=2?
Comment 7 Nick Clemens 2017-10-26 14:48:10 UTC
Created attachment 68617 [details] [review]
Bug 19462: Add a koha-elasticsearch command

This patch adds a new command to be used for Elasticsearch-related
tasks.

The current implementation only offers the --rebuild action switch,
that allows reindexing Elasticsearch on a per-instance basis as we are
used to with the rest of the koha-* commands.

Other options could be added in a future: --status (ES server status
report, etc).

To test:
- Apply the whole patchset
- Have a suitable Koha+Elasticsearch setup [1]
- Run:
  $ reset_all
- Run:
  $ man koha-elasticsearch
=> SUCCESS: A pretty man page is displayed covering all options
- Run:
  $ sudo koha-elasticsearch blah
=> SUCCESS: The script fails because blah is not a valid instance name
- Run:
  $ sudo koha-elasticsearch -v kohadev
=> SUCCESS: Reindex happens!
- Try the -a, -b and -c option switches
- Sign off happily :-D

Sponsored-by: ByWater Solutions

[1] This is straightforward if you are using KohaDevBox and created the
box using: $ KOHA_ELASTICSEARCH=1 vagrant up

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Nick Clemens 2017-10-26 14:48:13 UTC
Created attachment 68618 [details] [review]
Bug 19462: Add koha-elasticsearch documentation

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Nick Clemens 2017-10-26 14:49:02 UTC
More options can be added in the future, but this will ease testing and is a great start
Comment 10 Mark Tompsett 2017-10-26 16:24:11 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> (In reply to M. Tompsett from comment #5)
> > For example, try a "-c=2" instead of a "-c 2". Ugliness ensues.
> 
> Are we supposed to support -c=2?

Not expressly. But sometimes, some scripts are that format, and sometimes they are not. I'm not sure which is the officially preferred way for scripts in general, but I thought I would mention it as a valid weakness, since for some strange reason I typed that instead of what was expected and documented.
Comment 11 Tomás Cohen Arazi 2017-10-26 16:54:53 UTC
(In reply to M. Tompsett from comment #10)
> (In reply to Tomás Cohen Arazi from comment #6)
> > (In reply to M. Tompsett from comment #5)
> > > For example, try a "-c=2" instead of a "-c 2". Ugliness ensues.
> > 
> > Are we supposed to support -c=2?
> 
> Not expressly. But sometimes, some scripts are that format, and sometimes
> they are not. I'm not sure which is the officially preferred way for scripts
> in general, but I thought I would mention it as a valid weakness, since for
> some strange reason I typed that instead of what was expected and documented.

I don't think we should take care of non-documented ways of passing parameters. Either the manpage or the --help switch propose that syntax.

I don't even think we should support multiple ways of passing the parameters, as it tends to this: confussion.
Comment 12 Jonathan Druart 2017-10-27 17:16:54 UTC
Pushed to master for 17.11, thanks to everybody involved!