Implement build OS parsing into ImageVersionOS#184
Conversation
Test Results444 tests 444 ✅ 1m 26s ⏱️ Results for commit c819cc2. ♻️ This comment has been updated with latest results. |
bschwedler
left a comment
bschwedler
left a comment
There was a problem hiding this comment.
Thanks for doing this!
| ("Ubuntu 22", SUPPORTED_OS["ubuntu"]["22"]), | ||
| ("Ubuntu 24.04", SUPPORTED_OS["ubuntu"]["24"]), | ||
| ("Ubuntu 24", SUPPORTED_OS["ubuntu"]["24"]), | ||
| ("Ubuntu", SUPPORTED_OS["ubuntu"]["24"]), |
There was a problem hiding this comment.
I notice this is the only test that defaults to the latest of a version isn't specified. The code looks like it will do this for other OSes as well.
Should we add some tests for the other OSes?
Yes, I know we will have to keep the test up-to-date with the config, but I think that's a good thing.
There was a problem hiding this comment.
Thank you for noticing this. I was just kind of adding them willy-nilly to cover all the perceived combinations. This made me realize that the way I was grabbing the latest version was using the max string, not the max integer.
| # If only the name is provided and it's an unversioned OS, use it. | ||
| if isinstance(SUPPORTED_OS.get(match_dict["name"]), BuildOS): | ||
| return SUPPORTED_OS[match_dict["name"]] | ||
| # Otherwise, use the latest version of the matching OS name if possible. | ||
| else: | ||
| latest = max(SUPPORTED_OS[match_dict["name"]].keys()) | ||
| return SUPPORTED_OS[match_dict["name"]][latest] |
There was a problem hiding this comment.
It took me a bit to understand how this worked, but I now get that we skip the version key in the dict for OSes that we aren't listing specific versions
ianpittwood
force-pushed
the
build-os-impl
branch
from
85154d2 to
146406d
Compare
Fix string sort to int sort for finding latest version
2 participants
Stacks on #182