updates to the fallback onto LOVD error messages and warnings, see ht…

[Peter-J-Freeman]

…tps://github.com//issues/737

[Peter-J-Freeman]

Hi @John-F-Wagstaff. Updatates to the env and also LOVD API fallback included in this PR. Please chack it over

[John-F-Wagstaff]

Hello Pete

This looks OK, code wise and the normal pytests all pass on my system, but I do have a few comments

Firstly, for the install requirements, we have set more to ==. This is not necessarily wrong but it is something we are going to need to keep an eye on, and have some kind of system/schedule to check and update. If we don't then we could get locked to old, even archaic versions over time, particularly when the compatibility limits of one older component locks out newer versions of others, basic shared dependencies can be surprisingly incompatible between versions in python.

Secondly the Dockerfile updates look valid, but given how it is used I can't directly test without getting a local jenkins version set up, I tried using jenkinsfile-runner but it had version compatibility issues with the java libraries on my system so no luck on a more partial test. The code changes match the working version for the API though so it should be fine, but if you want me to run a local test I will need a bit more time.

Thirdly we might want to be able to prevent fallback to LOVD web. Silent fallback to the web version when the local version is installed but not working could hide issues in the future. If we do then we may want to put LovdSyntaxcheckLibraryVersion (and or the base URL for the LOVD check) into our metadata, rather than putting it into the warnings. This sligtly expands the effects of this patch, and might be out of scope for this pull request, but it is something we might want to think about later, if so.

The last, possibly nit-picky issue is that you have merged patches for 2-3 unrelated issues into 1. It would be more neat to keep the install requirements and Docker work in a separate patch to the code changes, separating by function like this can make finding errors by git-bisecting or other such processes simpler and is generally encouraged even if it does not affect the actual changes suggested.

[Peter-J-Freeman]

Hi @John-F-Wagstaff. The == sections were a vain attempt to lock into fast ans stable versions of some modules. I will remove these and re-test.

Don't worry about the docker install. It is rarely used and I think we may remove it and only allow the rest api to be built through docker since it makes more sense and does contain a complete VV build anyway. I can get it running locally.

The last, possibly nit-picky issue is that you have merged patches for 2-3 unrelated issues into 1. It would be more neat to keep the install requirements and Docker work in a separate patch to the code changes, separating by function like this can make finding errors by git-bisecting or other such processes simpler and is generally encouraged even if it does not affect the actual changes suggested.

Agreed, this was caused by lack of coffee, too much rushing and other distractions :)

Thirdly we might want to be able to prevent fallback to LOVD web. Silent fallback to the web version when the local version is installed but not working could hide issues in the future. If we do then we may want to put LovdSyntaxcheckLibraryVersion (and or the base URL for the LOVD check) into our metadata, rather than putting it into the warnings. This sligtly expands the effects of this patch, and might be out of scope for this pull request, but it is something we might want to think about later, if so.

I will push up a new env build tested for now. Can you pull and add in this bit of code? will add a note here once its good to go

[Peter-J-Freeman]

@John-F-Wagstaff the upodated env is in place

[Peter-J-Freeman]

plus this knocked a minute off the test time. So much for trying to set fast versions