Validate tljh specific config

[jrdnbradford]

This is a stab at the FIXME in tljh/config.py for adding config.yaml validation. It uses jsonschema to validate the changes against it.

This is a work in progress; I've tried to use the tljh-config docs and configurer.py to fill it out the best I can.

Example of some current validation:

sudo tljh-config set user_environment.default_app jupyterlb
'jupyterlb' is not one of ['jupyterlab', 'classic']

Certainly not perfect, looking for input before I go any further! 🚀 Also trying to get these tests to pass.

[jrdnbradford]

Looks like I've got most tests passing now.

[consideRatio]

Excellent work @jrdnbradford!

How should this behave if existing TLJH installations upgrade to get this validation? They could have invalid config already in place, and then making an unrelated config change may trigger a validation failure.

I'm not confident on what I think makes sense to do here. If we could, I'd like to error loudly and early on before we have actually upgraded anything. I'm not sure we can do that while making use of this validation logic inside the tljh python package, as then we have already upgraded to use it I figure.

At the same time, maybe not. The config could have been manually edited anyhow, and we should assume someone may have done that also...

[minrk]

One option to trial this is to have a flag to turn validation warnings into errors or vice versa (e.g. --no-validate for opt-out, or --strict for opt-in).

[jrdnbradford]

@minrk is the suggestion something like this:

• add --no-validate and --strict flags for tljh-config CLI command
  • --no-validate will be the default and will allow edits that don't validate against the schema, but will print a warning
  • --strict will not allow edits that fail to validate and will exit with an error

Those with invalid configs who update TLJH will start to get warnings when they use tljh-config but won't get any breaking behavior.

We could also do something like:

parser.add_argument("--validate", action = argparse.BooleanOptionalAction)

which should handle --validate and --no-validate in a similar way.

I'm open to whatever you all think is best and will edit and test after feedback,

[minrk]

--[no-]validate sounds like a good plan (yes, the names should definitely match if we give flags for both ways), thanks @jrdnbradford!

I also think it's just fine to validate by default if that's what others would prefer. I wouldn't call this breaking, because the validation error message should include a message showing how to re-run without validation, along the lines of:

You can still apply this change without validation by re-running with --no-validate (whole copy-pasteable command-line if it's easy).
if you think this validation error is incorrect, please report it to https://github.com/jupyterhub/the-littlest-jupyterhub/issues

[consideRatio]

I agree on the idea to validate by default, so having a flag to opt out of the validation is my preference.

Flag naming decision, here are some ideas:

• --no-validate
• --no-validation
• --no-config-validation
• --skip-validation
• --skip-config-validation

I'm leaning for the most verbose flags of either --no-config-validation or --skip-config-validation which clarifies that its about config validation specifically - because one could imagine other kinds of validation be made as well otherwise.

@minrk @jrdnbradford what do you think?

[minrk]

I like the verb forms, and I think it's best to stick the very standard no- prefix, and not uncommon skip-. So I'd go with --[no-]validate, or if you think it's really likely that we'll have more than one optional thing to validate, --[no-]validate-config.

[jrdnbradford]

Got an initial working version using --[no-]validate. The one failed integration test is because Python v3.8 doesn't have the argparse.BooleanOptionalAction option.

I also bumped the Dockerfile image to 22.04.

[jrdnbradford]

@consideRatio and @minrk if the failed Python 3.8 test is a blocker, I can rewrite to make it compatible. Let me know!

[minrk]

Yes, that would be great if you can make it work on 3.8. Thank you!

[jrdnbradford]

@consideRatio and @minrk the Python 3.8 test is now passing after removing argparse.BooleanOptionalAction. I also fixed the two unit tests that were failing (they were just missing pip so they couldn't install dependencies). Not sure if the Integration tests / Ubuntu 22.04, Py 3.10, from 0.2.0 (pull_request) test is still needed now that TLJH is 1.0.0. If not, I'm happy to remove it to get straight ✅ s across the board.

Let me know if I can do anything else here. 🚀

[consideRatio]

I think it could be nice to default functions to validate=True, but its a refactoring detail really.

I'm looking into the remaining test failure - which shows up in the main branch as well, so i think we can merge this anyhow.

[consideRatio]

@jrdnbradford a trick to combine multiple code suggestions is to visit "files changed", from there you can "add suggestion to batch" and add multiple code suggestions in one go.

I'm actively debugging the remaining test failures now btw

[jrdnbradford]

@jrdnbradford a trick to combine multiple code suggestions is to visit "files changed", from there you can "add suggestion to batch" and add multiple code suggestions in one go.

I'm actively debugging the remaining test failures now btw

Ah, good tip, sorry for the spam. 😃

[jrdnbradford]

Refactoring done. Good to merge when you're ready. Thanks, everyone!

[consideRatio]

THANK YOU @jrdnbradford!!!

I find robustifying contributions like these critical for the health of a open source projects - thank you ❤️ 🎉 🌻!!