Use id from erc.yml #45

LukasLohoff · 2018-03-27T10:55:58Z

The loader now uses the ID specified in erc.yml (#44 ) for compendia:

Reads ID from erc.yml
Checks for validity and if a compendium with the ID already exists
Added tests for invalid ID & duplicate ID
Reworked tests to reset database and local compendium storage before each test

nuest · 2018-03-27T12:35:24Z

lib/steps.js

+
+            } catch (error) {
+                debug('[%s] Could not read compendium detection file %s: %s', passon.id, passon.detectionFile, error);
+                error.msg = 'Could not read compendium detection file';


You're not reading the compendium detection file, but the "compendium configuration file", see http://o2r.info/erc-spec/spec/#erc-configuration-file

Before it was only used for detection. Please rephrase.

nuest · 2018-03-27T12:39:17Z

lib/steps.js

+                reject(error);
+            } else {
+                // Validate compendium id
+                let idRegex = new RegExp(config.bagtainer.id_regex);


I think new RegExp is not neccessary, you can directly do config.bagtainer.id_regex.test(compendiumID).

nuest · 2018-03-27T12:42:59Z

lib/steps.js

 const Stream = require('stream');
 const meta = require('../lib/meta');
+const yaml = require('js-yaml');
+const randomstring = require('randomstring');


randomstring is not used in this file AFAICS

nuest · 2018-03-27T12:43:30Z

lib/steps.js

+                            reject(err);
+                        }
+                        if (compendium === null) {
+                            debug('[%s] Assigned new ID %s to compendium', passon.id, compendiumID);


Please clarify here: you are assigning the id loaded from the configuration file, not a "new" one.

nuest · 2018-03-27T12:44:55Z

lib/steps.js

+                    debug(error, stderr, stdout);
+                    let errors = error.message.split(':');
+                    let message = errorMessageHelper(errors[errors.length - 1]);
+                    error.msg = 'moving compendium files to new location failed: ' + message;


Have you thought about what do do with the files on an error here? Should they be deleted?

I don't think it's necessary to delete the directory as it's just a directory with a randomly generated name (from the ID)

nuest · 2018-03-27T12:45:30Z

lib/steps.js

+            let cmd = 'mv ' + passon.compendium_path + ' ' + updatedPath;
+            exec(cmd, (error, stdout, stderr) => {
+                if (error || stderr) {
+                    debug(error, stderr, stdout);


Please add a short message to this debug statement. Printing all outputs is good, a short text helps identifying where the error happens.

nuest · 2018-03-27T12:46:12Z

package.json

 {
  "name": "o2r-loader",
-  "version": "0.10.0",
+  "version": "0.10.1",


This is more than just a bugfix, please set version to 0.11.0

nuest · 2018-03-27T12:57:48Z

The tests on Travis fail (https://travis-ci.org/o2r-project/o2r-loader/builds/358827922):

/loader/node_modules/bindings/bindings.js:88
        throw e
        ^
Error: Error relocating /loader/node_modules/detect-character-encoding/build/Release/icuWrapper.node: utrace_cleanup_59: symbol not found
    at Object.Module._extensions..node (module.js:681:18)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Module.require (module.js:596:17)
    at require (internal/module.js:11:18)
    at bindings (/loader/node_modules/bindings/bindings.js:81:44)
    at Object.<anonymous> (/loader/node_modules/detect-character-encoding/index.js:3:37)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Module.require (module.js:596:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/loader/lib/steps.js:34:33)

@LukasLohoff You did not change the Dockerfile, so this surprises me... it seems to be the same issue we had with sonicdoe/detect-character-encoding#8

try out locally with with the current base image node:8-alpine
try out locally with base image node:9-alpine (should not make a different, both use alpine:3.6!)
try out locall with node:8-slim

nuest · 2018-03-27T15:52:27Z

node:8-slim seems to have worked, but now we have troubles with the test setup, the test code cannot delete the files created by the microservice containers because the travis user is not root...

nuest · 2018-03-27T16:18:18Z

Dockerfile

-
-RUN apk add --no-cache \
+RUN apt-get update && apt-get install -y \
+    python \


Does this install Python 2 or 3? IIRC this is used for npm package bagit, just in case one of the tests with bags fails now.

It installs python 2. bagit should work, I verified that in a separate node:8-slim container

LukasLohoff added 12 commits March 22, 2018 16:39

read id from compendium file for compendia

494e78c

bump minor version

42407ef

add test cases for existing id and invalid id in compendium

89c87f3

add test ERC with invalid id

c6e0ac1

update readme with info about public sciebo ERCs

3e5a3e2

check for existing compendium id when uploading

2f40090

fix error messages

1e6c982

delete compendia before each test

e9ce413

update tests

d5d8146

udpate controllers for id detection

936f292

fix tests

398c024

delete compendium database and storage before tests

abdc520

nuest suggested changes Mar 27, 2018

View reviewed changes

update slack dependency to 11.0

b808ace

LukasLohoff added 4 commits March 27, 2018 14:58

increase major version

6311e26

rename ERC detectionFile to configurationFile

ac63a6b

update steps.js with PR suggestions by @nuest

85a1b3c

fix dockerfile

645f643

nuest approved these changes Mar 27, 2018

View reviewed changes

nuest reviewed Mar 27, 2018

View reviewed changes

LukasLohoff added 3 commits March 28, 2018 15:09

fix permission issue when deleting compedia

ed3203a

remove describe.only

4d11837

inc loader timeout for sciebo

c31a666

nuest merged commit 2195b01 into o2r-project:master Mar 28, 2018

Use id from erc.yml #45

Use id from erc.yml #45

Uh oh!

Conversation

LukasLohoff commented Mar 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nuest commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nuest commented Mar 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nuest commented Mar 27, 2018 •

edited

Loading