fix: improve platforms.<platform>.build-on validation
#633
Conversation
|
For the record, I am also open to modifying the Pydantic spec for |
rockcraft/models/project.py
Outdated
| @classmethod | ||
| def _helpful_error_for_non_list_build_on(cls, val: list[str]|Any) -> list[str]: | ||
| """Provide helpful error when 'build_on' is not a list.""" | ||
| if not isinstance(val, list): |
There was a problem hiding this comment.
thanks for the PR. Note that val could also validly be None here
regardless, I think the right place to fix this issue is where handle the platform error, so that it's more general and applies to all platform fields.
There was a problem hiding this comment.
Thanks for taking a look!
Note that val could also validly be None here
This validation hook only gets called if builds-on: is actually specified in rockctaft.yaml so this will not cause any problems whatsoever if builds-on: is omitted entirely.
The official documentation only specifies list[str], so from my PoV, both a bare build-on: <nothing>, as well as explicit build-on: null are clear spec violations which warrant an explicit error.
regardless, I think the right place to fix this issue is where handle the platform error
The Platform class gets instantiated right above that line, at which point its validators get run, so by that stage the erroneous error message is already there and the damage already done.
There was a problem hiding this comment.
This validation hook only gets called if builds-on: is actually specified in rockctaft.yaml so this will not cause any problems whatsoever if builds-on: is omitted entirely.
That might be but there are many tests failing with this new check; e.g. test_image_service_cache
The Platform class gets instantiated right above that line, at which point its validators get run, so by that stage the erroneous error message is already there and the damage already done.
The error_dict object has the location of the error, so it should be possible to provide a better message with the info that's already there.
There was a problem hiding this comment.
That might be but there are many tests failing with this new check; e.g. test_image_service_cache
Weird, I can vouch that running it e2e on a rockcraft.yaml file doesn't present any issues so from my PoV it means the tests are either rigged up incorrectly with respect to the real world, or they are generally not conclusive and should be improved.
I will change it to isinstance(val, (list, type(None)))now to see if that satisfies the tests FWIW.
regardless, I think the right place to fix this issue is where handle the platform error,
The error_dict object has the location of the error, so it should be possible to provide a better message with the info that's already there.
I still believe this will lead to validation details of sub-fields "bleeding" up the stack which should be avoided.
I'm unfortunately very short on cycles nowadays and am unable to address this now; feel free to close the PR and address the issue in the future if you see fit!
ef68bc6 to
3e2bb52
Compare
|
Just noticed the formatting issues and fixed them now, feel free to re-run the checks at your leisure! |
Provide a helpful error message if `rockcraft.yaml` contains a `platforms.<platform>.build-on` which is not a list. Fixes canonical#632. Signed-off-by: Nashwan Azhari <nazhari@cloudbasesolutions.com>
3e2bb52 to
95dd5c9
Compare
|
Coincidentally fixed by #567. Closing this PR now, thank you for everything! |
Provide a helpful error message if
rockcraft.yamlcontains aplatforms.<platform>.build-onwhich is not a list.Fixes #632.
* my employing organization should have signed it AFAIK.
Example of change
Given this snipped of a
rockcraft.yaml:Before:
After: