-
Notifications
You must be signed in to change notification settings - Fork 149
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
base: main
Are you sure you want to change the base?
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 |
f31629b
to
46c3981
Compare
./scripts/validate-autoinstall-user-data is used by the integration tests to verify the written 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). This changes the validation script to rely on the logic from the server directly to perform pre-validation of the the supplied autoinstall configuration. Additionally, this adds an argparser to make it more user-friendly. Now we can advertise this script as something for users to pre-validate their autoinstall configurations.
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.
./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