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

misc: add Containerfile #11

Merged
merged 6 commits into from
Dec 19, 2024
Merged

misc: add Containerfile #11

merged 6 commits into from
Dec 19, 2024

Conversation

mvo5
Copy link
Collaborator

@mvo5 mvo5 commented Dec 13, 2024

This PR adds Containerfile integration, with that a bib style deploy/run is possible. The Containerfile is pretty nasty right now, especially around the "getting supported repositories" part. This needs to get fixed either in composer or images IMHO (or via go:embed). But with that we can give it to our early users and let them play with building images.

It also contains automatic container building as part of the GH action (thanks @supakeen!) and once we have it we can update the README to point to the auto-generated containers for even easier testing/using.

As is "list-images" and "manifest" will work but once #8 is merged the below works (on my local system, we want tests here :)

Run build via container, bib-style:

$ sudo podman build . -t test-icbli
$ sudo podman run --privileged \
   -v ./output:/output \
   test-ibcli:latest \
   build \
   --distro fedora-41 \
   minimal-raw

@supakeen
Copy link
Member

supakeen commented Dec 14, 2024

I've been rude and committed CI, and a path fix for the container build, we might need to change organisation or repository permissions for it to upload.

@mvo5 mvo5 force-pushed the add-containerfile branch from e3da24b to 2ca63b4 Compare December 16, 2024 09:01
@mvo5 mvo5 marked this pull request as ready for review December 16, 2024 09:01
supakeen
supakeen previously approved these changes Dec 16, 2024
Copy link
Member

@supakeen supakeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good (though I guess I wrote part of it so maybe wait for a second review).

Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it! I have a few concerns about the entrypoint, and I would love to see at least a quick smoke test of the container image.

Containerfile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
entrypoint.sh Outdated
mkdir /run/osbuild-store

mount -t tmpfs tmpfs /run/osbuild
mount -t tmpfs tmpfs /run/osbuild-store
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is currently unused, and I think we should actually use a "real" filesystem for the osbuild store, because tmpfs might not be big enough for image building.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, indeed, I removed this and followed our pattern from bib and added --store and use that in the entrypoint.

Containerfile Outdated Show resolved Hide resolved
Containerfile Show resolved Hide resolved
.github/workflows/container.yaml Show resolved Hide resolved
@supakeen
Copy link
Member

I've made 3 separate commits to take care of the low hanging fruit things, feel free to reorganize/rebase how you see fit @mvo5

(I don't want to force push to your branch at all).

The other remarks will take a bit more thinking.

mvo5 and others added 3 commits December 18, 2024 10:57
This commit adds a new `--store` flag to the `image-builder build`
command. This is used to specify a osbuild store directory to
store the intermediate build tree for faster rebuilding. It is
also useful for the container version of `image-builder-cli` to
allow mapping the users store into the container.
This commit adds an initial Containerfile to build an `image-builder`
container.

Use all the DNF commands together and add a cleaning command to the end
of it. This makes fewer layers and keeps the image size slightly
smaller.

Co-authored-by: Simon de Vlieger <supakeen@redhat.com>
Signed-off-by: Simon de Vlieger <supakeen@redhat.com>
Automatically build the container for `image-builder-cli` and upload it
to `ghcr.io`. This builds only for x86 initially.

Signed-off-by: Simon de Vlieger <supakeen@redhat.com>
@mvo5 mvo5 force-pushed the add-containerfile branch 3 times, most recently from 88d35d0 to ceefc4a Compare December 18, 2024 11:37
This commit adds a smoke test that builds the ibcli container
and runs a fedora-41 raw-minimal build to double check that
the container based building actually works.

Thanks to Ondrej for suggesting this.
@mvo5 mvo5 force-pushed the add-containerfile branch from ceefc4a to 7527f39 Compare December 18, 2024 11:43
mvo5 added 2 commits December 18, 2024 13:10
This commit adds a new workflow that runs the new `pytest` based
integration tests inside GH actions. It also extracts a common
`testdeps.yml` reusable workflow so that we do not duplicate the
package list of test dependencies.
This commit creates a temporary directory for the defaultDepvolver()
if no cacheDir is given. This ensures that we do not clutter the
current working directory with `platform:foo` cache directories
that `solver.Depvolve()` creates.

Note that this is only tested indirectly via the integration test
in `test_container.py:test_container_builds_image()` that checks
that the output directory is clean.
@mvo5 mvo5 force-pushed the add-containerfile branch from 7527f39 to 5a468cd Compare December 18, 2024 12:10
@mvo5
Copy link
Collaborator Author

mvo5 commented Dec 18, 2024

I've made 3 separate commits to take care of the low hanging fruit things, feel free to reorganize/rebase how you see fit @mvo5

(I don't want to force push to your branch at all).

The other remarks will take a bit more thinking.

Thank you! This is really appreciated! I squashed them into their relevant places (hopefully :) and also addressed the points from @ondrejbudai - I think this is ready for a re-review. I can split out further if needed but it seems it's not toooo big .

Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@achilleas-k
Copy link
Member

One thought about the new --store flag (to consider in a follow-up):
Would it make sense to separate osbuild flags into their own group? Could we have an --osbuild-opts flag for example that can take options to be passed down to osbuild?

So we could do:

image-builder-cli ... --osbuild-opts="store=/data/osbuild-store,checkpoint=build,checkpoint=os,cache-max-size='2 GiB'" ...

It's not the prettiest but it saves us from having to support every osbuild flag. OTOH, it also forces users to look up osbuild flags if they want to discover everything that's supported.

If we don't like this, I think eventually we should support every option that osbuild supports (except maybe the --monitor option).

@mvo5 mvo5 enabled auto-merge December 19, 2024 05:59
Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I have some suggestions, but nothing that will cause the world to burn, so let's merge this in order to iterate quickly, and have this under the christmas tree. :) 🎄

@@ -13,6 +13,7 @@ jobs:

build:
runs-on: ubuntu-latest
uses: ./.github/workflows/testdeps.yml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

test/containerbuild.py Show resolved Hide resolved
@@ -0,0 +1,27 @@
import os
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be willing to use Go for integration tests? Is there any reason to actually use Python?

I can tell you why I prefer Go:

  • developers don't need to switch their language mindset when writing integration tests
  • we don't need an extra linter and formatter
  • sharing code between integration tests and the SUT
  • pytest fixtures are cool... until they start being overwhelming. I think I have a preference for the lightweight nature of Go.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will give this a "go" (see what I did here ;)

Containerfile Show resolved Hide resolved
test/test_container.py Show resolved Hide resolved
.github/workflows/pytest.yml Show resolved Hide resolved
@mvo5 mvo5 added this pull request to the merge queue Dec 19, 2024
Merged via the queue into osbuild:main with commit 058e3d6 Dec 19, 2024
1 check passed
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.

4 participants