-
Notifications
You must be signed in to change notification settings - Fork 155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update validate-autoinstall-user-data script #1901
Update validate-autoinstall-user-data script #1901
Conversation
722fc30
to
d8d9f0c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far. Use of dry run here is interesting. We do occasionally have bugs where dry run really runs commands but I don't consider that blocking.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks nice overall. A few comments though.
|
||
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, let's think about it later. This is not blocking.
17ace31
to
77ab65d
Compare
Updated + rebased onto main. Thanks for the feedback @ogayot |
46c3981
to
5413f4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once we can make the CI happy.
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.
5413f4d
to
7ab4296
Compare
Finally coming back to this. A couple of things:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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