Update validate-autoinstall-user-data script #1901
Conversation
Chris-Peterson444
changed the title
Chris-Peterson444
force-pushed
the
user-data-validation
branch
2 times, most recently
from
722fc30
to
d8d9f0c
Compare
Chris-Peterson444
force-pushed
the
user-data-validation
branch
3 times, most recently
from
691476d
to
6a3abc7
Compare
|
I think this is ready. It's not perfect but it's a good start. I think I would like to punt writing tests for this since it uses the dry-run logic that is already tested. Eventually I think we could use this in CI for some more testing, which at that point we could write some more tests for this particular script if we wanted. Eventually I would like to package this (and subiquity as a whole) in a way that users could just install the subiquity snap and invoke the validator from there, which would reduce the setup here. |
|
|
||
|
|
||
| SUCCESS_MSG = "Success: The provided autoinstall config validated succesfully" | ||
| FAILURE_MSG = "Failure: The provided autoinstall config did not validate succesfully" |
There was a problem hiding this comment.
nitpick: I'd reword just a bit so that the last words written are not "validate successfully". I'm a lazy reader 😆
The provided autoinstall config failed validation?
There was a problem hiding this comment.
Also, I'm tempted to suggest rich (https://rich.readthedocs.io/en/stable/introduction.html) but it's probably difficult to justify for only one message.
There was a problem hiding this comment.
lazy reader
Me too. I couldn't come up with something at the time but I like it. I used your suggested text.
I'm tempted to suggest
rich
I do like the idea. And it's in Main, which is good. Although maybe once (if) we start having more error messaging we could use it?
There was a problem hiding this comment.
yeah, let's think about it later. This is not blocking.
Chris-Peterson444
force-pushed
the
user-data-validation
branch
2 times, most recently
from
17ace31
to
77ab65d
Compare
|
Updated + rebased onto main. Thanks for the feedback @ogayot |
Chris-Peterson444
force-pushed
the
user-data-validation
branch
3 times, most recently
from
46c3981
to
5413f4d
Compare
In the future we want to use dry-run and subiquity internals to do more robust validation of autoinstall user-data. Today the CI isn't ready for this and we should rely on old behavior to not regress CI results. This effectively moves current behavior behind the --legacy flag.
With moving to make the validation script more user facing, we don't need users to have the documentation link in their autoinstall file. Add a hidden flag, --check-link, to be used in CI to validate rendered autoinstall config has the documentation link.
./scripts/validate-autoinstall-user-data is used by the integration tests to verify the rendered user data validates against the combined JSON schema, but we have introduced run-time checks for more things than can be caught by simple JSON validation (e.g. warns/errors on unknown keys or strict top-level key checking for supporting a top-level "autoinstall" keyword in the non-cloud-config delivery scenario). Now the validation logic relies on the server validation logic directly to perform pre-validation of the the supplied autoinstall configuration.
Chris-Peterson444
force-pushed
the
user-data-validation
branch
from
5413f4d
to
7ab4296
Compare
|
Finally coming back to this. A couple of things:
|
There was a problem hiding this comment.
An interesting challenge with the run-time verifier is that it has assumptions that may not match the target installation ISO.
Examples, which can be valid but will fail validation:
- missing identity / user-data - would be valid on Desktop
- source asking for anything but
id: synthesized- might be valid, if the id matches install-sources.yaml on the ISO
I feel like the validator is still useful but I think it would be worth mentioning these limitations, if we haven't already.
Chris-Peterson444
force-pushed
the
user-data-validation
branch
from
7ab4296
to
79217f4
Compare
|
Great point. This is one of the issues I was trying to avoid in CI for now and we should definitely document it. How does this look? I added a new section "Validator Limitations" and tried to address all of the things I might consider as limitations of the validator (mostly rearranging the disclaimers I had put in the intro previously), including the points you provided. |
./scripts/validate-autoinstall-user-data is used by the integration tests to verify the written user data is correct, but we can additionally advertise this script as something for users to pre-validate their autoinstall data.
Changes include some refactoring and trying to catch some likely common mistakes.
Todo:
[ ] Tests