Skip to content
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

Merged
merged 20 commits into from
Aug 9, 2024
Merged

Conversation

upils
Copy link
Collaborator

@upils upils commented Jul 31, 2024

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

@upils upils self-assigned this Jul 31, 2024
@upils upils force-pushed the make-imagecraft-build-an-image branch 2 times, most recently from dbe2575 to 402b981 Compare July 31, 2024 09:28
upils added 5 commits July 31, 2024 14:30
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]>
This is was is expected by ubuntu-image in the revision field.

Signed-off-by: Paul Mars <[email protected]>
@upils upils force-pushed the make-imagecraft-build-an-image branch from af92087 to e13f50b Compare July 31, 2024 12:33
Copy link

codecov bot commented Jul 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.63%. Comparing base (430be67) to head (2a5326b).
Report is 59 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

@upils
Copy link
Collaborator Author

upils commented Aug 5, 2024

@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.

docs/howto/basic_image.rst Outdated Show resolved Hide resolved
docs/howto/basic_image.rst Outdated Show resolved Hide resolved
docs/howto/basic_image.rst Outdated Show resolved Hide resolved
docs/howto/basic_image.rst Outdated Show resolved Hide resolved
docs/howto/basic_image.rst Outdated Show resolved Hide resolved
docs/howto/basic_image.rst Outdated Show resolved Hide resolved
docs/howto/basic_image.rst Outdated Show resolved Hide resolved
docs/howto/basic_image.rst Outdated Show resolved Hide resolved
docs/howto/basic_image.rst Outdated Show resolved Hide resolved
docs/howto/basic_image.rst Outdated Show resolved Hide resolved
@upils upils force-pushed the make-imagecraft-build-an-image branch from 6e219e1 to 6da0706 Compare August 5, 2024 14:36
upils added 12 commits August 5, 2024 17:04
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]>
@upils upils force-pushed the make-imagecraft-build-an-image branch from e3f368f to 8323a90 Compare August 5, 2024 15:08
@upils upils requested a review from sil2100 August 5, 2024 15:30
Copy link
Contributor

@sil2100 sil2100 left a 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.

docs/howto/basic_image.rst Outdated Show resolved Hide resolved
@@ -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
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

@upils upils Aug 8, 2024

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"
Copy link
Contributor

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.

Copy link
Collaborator Author

@upils upils Aug 8, 2024

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.

imagecraft/models/project.py Outdated Show resolved Hide resolved
@upils upils force-pushed the make-imagecraft-build-an-image branch from a4f1ad8 to 2a5326b Compare August 9, 2024 06:55
@upils upils requested a review from sil2100 August 9, 2024 07:37
Copy link
Contributor

@sil2100 sil2100 left a 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!

@sil2100 sil2100 merged commit 68b1ac1 into main Aug 9, 2024
13 of 14 checks passed
@sil2100 sil2100 deleted the make-imagecraft-build-an-image branch August 9, 2024 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants