-
Notifications
You must be signed in to change notification settings - Fork 67
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
cli: add index list option to init #192
base: master
Are you sure you want to change the base?
cli: add index list option to init #192
Conversation
16aa473
to
b0caec5
Compare
b0caec5
to
f1d5bf1
Compare
acf4f5b
to
cc3ec15
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I added a couple of small comments.
@@ -87,6 +87,38 @@ def test_init(app, template_entrypoints): | |||
|
|||
assert current_search_client.indices.exists(list(search.mappings.keys())) | |||
|
|||
# Test option --index-name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to test this!
A minor comment: does it make sense to maybe split in smaller tests?
assert 0 == result.exit_code | ||
assert current_search_client.indices.exists(list(search.mappings.keys())) | ||
aliases = current_search_client.indices.get_alias() | ||
assert 8 == sum(len(idx.get('aliases', {})) for idx in aliases.values()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a lot of issues in the past (on cds-videos
, another Invenio installation) with tests that contains these numbers, like 8 ==
, because when changing something, it is hard to know what number to put there and why such number.
Could you try to replace the number with a named constant or maybe explain in a comment?
@with_appcontext | ||
@es_version_check | ||
def init(force): | ||
def init(force, index_list): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of adding the default value?
def init(force, index_list): | |
def init(force, index_list=[]): |
or
def init(force, index_list): | |
def init(force, index_list=None): |
:returns: A string with the index name unsuffixed. | ||
""" | ||
search_ext = app.extensions['invenio-search'] if app else current_search | ||
suffix = suffix if suffix is not None else search_ext.current_suffix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suffixes are also timestamps, and if they are not matched, then init creates a new index, I think we should also include this in the test cases (I saw it happening on integration testing)
Add option to init to allow initialization of indices by name.
Usage:
$ invenio index init -i records-authorities-authority-v1.0.0 -i another-index
$ invenio index init -i index-a -i index-b