Bug 19462 - Add a koha-elasticsearch command
Summary: Add a koha-elasticsearch command
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Nick Clemens
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-13 15:29 UTC by Tomás Cohen Arazi
Modified: 2017-11-28 14:43 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
A new command-line script is added for handling Elasticsearch indexing-related tasks for each Koha instance.


Attachments
Bug 19462: Add a koha-elasticsearch command (6.45 KB, patch)
2017-10-13 18:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19462: Add koha-elasticsearch documentation (4.48 KB, patch)
2017-10-13 18:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19462: Add a koha-elasticsearch command (6.48 KB, patch)
2017-10-14 03:20 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 19462: Add koha-elasticsearch documentation (4.51 KB, patch)
2017-10-14 03:20 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 19462: Add a koha-elasticsearch command (6.54 KB, patch)
2017-10-26 14:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19462: Add koha-elasticsearch documentation (4.57 KB, patch)
2017-10-26 14:48 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 M. 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 M. 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 M. 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 M. 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!