Configure app development node version

[NoelDeMartin]

The node version to use in the mobile app is going to keep changing with new releases, so this value should be configurable.

[stronk7]

Hola!

A) Question: Are you going to use, always, numerical versions (14, 15, 18.12, xx.yy.zz... ) and never, never node versions like lts/gallium, in the .nvrmrc file?

Just asking because, if you can guarantee that numerical versions will be used always... then surely we don't need any grep facility and just put the contents of the .nvmrc file in the MOODLE_DOCKER_APP_NODE_VERSION and forget.

I mean, we don't need capturing groups, hence we can do the same for both unix and windows.

B) Also, instead of using any 14 (or whatever) as manual fallback both in the unix and windows compose script, I'd suggest to make it only once in the moodle-app-dev-ionic5.yml file, with something like this:

image: node:${MOODLE_DOCKER_APP_NODE_VERSION:-14}

That way we only have one place to change the default and we won't forget this or that compose scripts. We have followed recently that technique with other variables like MOODLE_DOCKER_DB_VERSION.

Ciao :-)

[NoelDeMartin]

Thanks @stronk7 for taking a look.

A) I can't guarantee that we'll always use a number version, but even if we do I just tried and it doesn't work because the version is written as v14.15.0 with a leading v. Although now that you mentioned it, I tried to see what happens using a named version and the script was throwing an error, so I fixed it to fall back to the default value.

B) Yeah good idea! I updated the code to do that.

I also noticed that this doesn't work with version: "2" of the composer files, but it worked if I changed all of them to version: "3". So I wondered why the master branch is passing in CI, and I found out the reason is that I didn't have the compose plugin installed. Once I installed it, it works properly. But it seems like the fallback to docker-compose is now broken (or maybe it depends on the version of docker-compose?).

Maybe I should open an issue about this, even if it's only to update the documentation?

[stronk7]

Thanks @NoelDeMartin,

all looks ok. Just let me try today if I can add some "number extractor from string" loop or whatever for the CMD version. I think I've seen some examples out there, simple enough, and that would allow us to keep both versions feature-paired.

About docker compose, yeah I thought that near every environment gets it by default (for sure Docker Desktop for Mac and Win does). Also GHA, and or own CIs... Around when compose v2 was introduced (or maybe before, don't remember the details), they decided to ditch the versions stuff in the compose files, that is now entirely optional / deprecated. I cannot look for the source of the information now, (it's related to they move to become the "standard" compose implementation, but it was documented, I'm sure I've read it.

I think the trick here is, to everybody facing any problem, to ensure that they are using a recent version of docker engine / docker cli. I would +1 to create an issue to clearly specify that a recent desktop/engine/cli are needed, yeah, good idea!

Ciao :-)

[stronk7]

Hi @NoelDeMartin,

I've added an extra commit to your branch bringing to the .cmd script with the ability to use the .nvmrc file in a similar way the unix one does. It only will be used it the information there is numerical.

I've tried it here and, so far, it works ok (same conditions, same app container launched...)...

With your thumbs up, I'm happy to merge this.

Ciao :-)

[NoelDeMartin]

Thank you @stronk7, I cannot test it myself because I don't have windows installed but it looks good to me. You can proceed with the merge :).

[stronk7]

Sold, thanks!