Skip to content
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: adds replace option #1887

Merged

Conversation

ioannistsanaktsidis
Copy link
Contributor

Signed-off-by: Ioannis Tsanaktsidis [email protected]

@tiborsimko
Copy link
Member

@ioannistsanaktsidis @pamfilos Tested the PR locally, works nicely, here are some observations for improvements:

1. Repetitive uploads. (important) if you load a file that has 100 records, say, and it fails in the middle, and you fix a problem for record number 50, then you cannot reload the file again, because:

  • using "insert" would lead to problems that PIDs already exist for existing records, and early exit;
  • using "replace" would work for only already-uploaded files, so no new records would be added.

You can try with:

$ cernopendata fixtures records \
    -f ./cernopendata/modules/fixtures/data/records/cms-primary-datasets.json \
    --skip-files --mode insert --verbose

that you would interrupt in the middle after say only 6 records are loaded.

=> Can you add an insert-or-replace option, for example allow multiple values --mode insert --mode replace that would do the job? Or for example make a new value --mode insert-or-replace?

The wanted behaviour is:

mode behaviour when PID exists behaviour when PID does not exist
insert fail upload as new
replace update as existing fail
insert or replace update existing upload as new

We basically need a new CLI option to express the 3rd scenario. Allowing multiple values --mode insert --mode replace could be one way; creating a new value --mode insert-or-replace could be another.

2. Output messages. (cosmetics) You could catch the exception invenio_pidstore.errors.PIDAlreadyExists and reply somehing more user friendly, like:

[ERROR] Record recid 15 exists already; cannot insert it.  
[ERROR] Record recid 15 does not exist; cannot replace it.

for the two "fail" situations in the above table.

You could also introduce informative messages:

[INFO] Record recid 15 inserted.
[INFO] Record recid 15 updated.

for the other situations in the above table, when things are OK and when the user used --verbose argument.

3. Non-record content. (future outlook) For now, doing this for records is enough. For future, it would be good to think about doing this for other fixtures, such as "docs", so that one could log into the PROD machine and upload only the latest given news document, without disturbing the others. Noting this down just for completeness; not necessary to address this now as part of this PR!

@ioannistsanaktsidis
Copy link
Contributor Author

@tiborsimko updated ! You can check now if it fits our case :) I think it covers all your comments.

 * Closes cernopendata#1836.

Signed-off-by: Ioannis Tsanaktsidis <[email protected]>
@tiborsimko
Copy link
Member

@ioannistsanaktsidis Great, everything looks OK!

@tiborsimko tiborsimko merged commit 280f7be into cernopendata:master Dec 2, 2017
@ioannistsanaktsidis ioannistsanaktsidis deleted the adds-replace-mode branch December 5, 2017 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants