nlbdev / nordic-epub3-dtbook-migrator

Tools for converting between a strict subset of DTBook and EPUB3.
http://nlbdev.github.io/nordic-epub3-dtbook-migrator/
GNU Lesser General Public License v2.1
8 stars 7 forks source link

Client and Web API of the validator #524

Closed kalaspuffar closed 1 year ago

kalaspuffar commented 1 year ago

Hi @josteinaj

This PR is an initial, work-in-progress package if the client is for the NordicValidator and the docker build for a web API.

Currently, the Dockerfile needs to be executed from the project root to create a new docker image. If we want that docker file could easily replace the file in the document root that builds Pipeline with migrator today.

Building the Client is not that hard, but installing the dependencies could be a bit fiddly. Maybe we need to add more to the README.md file to explain it in more detail. Was a while since I did it, but the Dockerfile has more information that could be used to get up and running.

Best regards Daniel

josteinaj commented 1 year ago

Thanks.

I didn't know cron jobs were possible in Docker now. Last time I checked it wasn't possible to do without giving the container root access to the system. I'll try something similar in other projects ;).

Could schxslt be installed through the Dockerfile? I think this should work:

# install schxslt
RUN wget https://github.com/schxslt/schxslt/releases/download/v1.9.4/schxslt-1.9.4-xslt-only.zip \
    && unzip schxslt-*.zip -d schxslt \
    && mv schxslt/schxslt-*/* schxslt \
    && rm schxslt-*.zip 

It would be good if all dependencies and compilation were done in the Dockerfile. I think that's necessary for building on Docker Hub anyway.

I've tried to build this but I can't get it working. I think it's something about the paths.

You can replace the main Dockerfile with the new one. We don't need two versions of the project. And yes, continuing the versioning would be good. Maybe even bump the major version since this is quite a big change (2.0.0).

I think it's fine if we only have a Docker image. We don't need to publish a maven package to Maven Central either, so feel free to remove any Maven stuff if you don't need it. The command line interface could maybe simply be the docker run command. That way we don't really need any install instructions other than "install Docker" and "run this command here". We could possibly have a convenience bash-script to make it easier to invoke from the command line.

kalaspuffar commented 1 year ago

Hi @josteinaj

I've updated it so that the main Dockerfile will be the new build. I've also added the script part you sent above so that schxslt will be downloaded during the build.

More over I added --no-epubcheck and --no-ace flags to the java client.

Last, I've added a feature we have in another project. It's a bit of a hack, but if you add the environment variable LOCAL_PATH when you deploy the Docker image, it will use that path for your files instead of fetching them from OneDrive. MTM had a similar request in another Docker API. Seems reasonable to keep the same solution. We could, of course, have two different API endpoints, but this will work in the same way and will not introduce security issues where users fetch files from directories they don't have access.

Best regards Daniel

josteinaj commented 1 year ago
~/nlb/nordic-epub3-dtbook-migrator @b584dd79 *5 ❯ docker build .                                                                                                         (…)
Step 18/58 : RUN mvn package
 ---> Running in 591647d733c5
[INFO] Scanning for projects...
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  0.059 s
[INFO] Finished at: 2022-11-01T11:21:35Z
[INFO] ------------------------------------------------------------------------
[ERROR] The goal you specified requires a project to execute but there is no POM in this directory (/opt/client). Please verify you invoked Maven from the correct directory. -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MissingProjectException
The command '/bin/sh -c mvn package' returned a non-zero code: 1
kalaspuffar commented 1 year ago

Hi @josteinaj

Seemed that the .gitignore file had *.xml for some reason. Now you should be able to build it.

Best regards Daniel

josteinaj commented 1 year ago

I tried it now and it builds, and it works!

I run it with docker run --rm -it -p 80:80 6eeb2fcde7db, and import the yaml definition of the API into Postman

The config parameter - could it be separate parameters instead? I think that would be easier in most cases, and it would probably be easier to document in the yaml file as well.

What are the next steps now? Do we still have tests? The pom.xml still references Pipeline 2, so I guess that needs to be changed?

josteinaj commented 1 year ago

The current tests seem to be in client/src/test/java/TestValid.java. Are they run during mvn package, so that the build fails if the tests fail?

Can I merge this PR or is there something else you'd like to do here before I do so?

kalaspuffar commented 1 year ago

No, you can merge if you like. There will probably be more added later but as a initial implementation I'm done.

kalaspuffar commented 1 year ago

No, you can merge if you like. There will probably be more added later but as a initial implementation I'm done.

karladamt commented 1 year ago

Do we need to look for the nav.ncx-file'? We are never including it, so maybe we should exclude that check. What do you think @josteinaj?

josteinaj commented 1 year ago

nav.ncx is not required in the 2020-1 guidelines, so we shouldn't require it to be there.

karladamt commented 1 year ago

Then could you please @kalaspuffar remove that check.

kalaspuffar commented 1 year ago

Hi @josteinaj and @karladamt

It's not required. It will not generate an issue with the validator, and the EPUB will pass. It is a notice so visible in the report, so you have that information as well. If you expected the file to be produced by partners and it's missing, this information is important.

Best regards Daniel