-
Notifications
You must be signed in to change notification settings - Fork 71
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
refactor: use craft-platforms for build plans #1894
Conversation
0ee54d6
to
e641da8
Compare
Only applies to `platforms` charms
platforms=platforms, | ||
) | ||
return [ | ||
models.BuildInfo( |
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.
suggestion: maybe craft-application's BuildInfo should have a staticmethod to convert a craft-platform's BuildInfo
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.
Both of your comments here are things I'd like to address in canonical/craft-application#391 :-)
# https://github.com/canonical/craft-platforms/issues/43 | ||
craft_platforms.Platforms, # pyright: ignore[reportPrivateImportUsage] | ||
{ | ||
name: (platform.marshal() if platform else None) |
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.
this is just me rambling but it bothers me a bit that we have to do this 'round-trip' where we unmarshal the project, which includes validation (necessary) and possibly transformation (dangerous), and then re-marshal to pass to craft-platforms. This makes the assembling of the build plan a little different between the application and systems that just use craft-platforms (like launchpad), and makes us still need "all" of this code here.
I don't have any good suggestions to address this though; maybe the base BuildPlanner class should retain the original values of base, build-base and platforms somewhere?
Only applies to
platforms
charms.