-
Notifications
You must be signed in to change notification settings - Fork 6
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
Make imagecraft build an image #53
Conversation
dbe2575
to
402b981
Compare
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
This should be installed in a specific way and cannot be simply a dependency declared in the pyproject.toml Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
This is was is expected by ubuntu-image in the revision field. Signed-off-by: Paul Mars <[email protected]>
af92087
to
e13f50b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #53 +/- ##
==========================================
+ Coverage 97.60% 97.63% +0.03%
==========================================
Files 20 20
Lines 500 507 +7
Branches 81 82 +1
==========================================
+ Hits 488 495 +7
Misses 10 10
Partials 2 2 ☔ View full report in Codecov by Sentry. |
@rkratky would you mind taking a quick look at the how to build a basic image in this PR? Sadly for now you cannot follow this how-to until this PR is merged, but as soon as it is I will ask someone from Foundations to try it to know what could be improved. |
6e219e1
to
6da0706
Compare
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Until we have a proper way to express it in the YAML file, set it to true to generate valid images for newer series. Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
e3f368f
to
8323a90
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.
This is great. In general it's a +1 - left a few inline comments but only the version->revision one IMO would require changing (and one docs change). Other than that, all good.
@@ -177,6 +178,7 @@ def __init__( # noqa: PLR0913 | |||
names=seed_names, | |||
pocket=seed_pocket, | |||
), | |||
sources_list_deb822=True, # Always set it to true for now |
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.
We'll have to figure out how to approach this from the imagecraft POV. Maybe simply keying it on the base would be sufficient.
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.
Yes, this is "hack" for now. The "base" thing is a really tricky thing because the craft framework is expecting this base to exists and to be usable to build. How do we build a newer series on an older one in this case?
But otherwise I agree we could probably look at some value in the provided YAML and decide for the user if this should be set True/False.
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.
Or we could use the version
field?
|
||
def list_image_paths(workdir_path: str) -> list[str]: | ||
"""Extract the list of images from a ubuntu-image.json file.""" | ||
p = pathlib.Path(workdir_path) / "ubuntu-image.json" |
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.
Let's go with this for now. Conceptually however I'd like us to think about it a bit more. The ubuntu-image.json
file is rather an 'internal' artifact, not really exported publicly, with no guarantees of fields staying the same etc. Conceptually it's not great to depend so much on internal implementation of a different tool. I agree it's a big plus side that we control it as we control ubuntu-image, but at the same time I can already see additional work needed everytime we change our state machine and what we save - at least from the test-case side. We'd have to update the test cases then to include new actual ubuntu-image state files.
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.
I also agree this is not ideal and is also a "hack". My first idea was to have ubuntu-image generate a JSON (or any other easily parse-able format) "report" that would contain the names of built artifacts. But this is very specific to the "pack" use case because otherwise the user already specifies it in the ubuntu-image.yaml.
We are only looking for a single key in the JSON and are not really checking the schema, so changes in the JSON should not impact this too much, I hope.
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
a4f1ad8
to
2a5326b
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.
Looking good! And there's some good ideas to discuss in the inline comments. Let's merge!
Several fixes to bugs found while making imagecraft able to build an image. Also add a basic how-to build an image with imagecraft.
Fixes: FR-8431