bib/internal/setup: Tag validate early #475

bearrito · 2024-06-13T23:37:27Z

This is my first attempted commit to this project.

I'm still learning the repo structure and dev workflow. Few questions on testing.

In this commit I'm relying on having images locally. I didn't see anything similar so I assume we want to avoid this/tests will fail. Is there an alternative?
If I have to rely on testing by hand, how can I use local changes? I'm thinking I just https://github.com/osbuild/bootc-image-builder?tab=readme-ov-file# building and then can follow examples?

Closes: #380

bearrito · 2024-06-15T21:03:20Z

Working, testing by hand

bearrito@home:~/Git/bootc-image-builder$ sudo podman run --rm  -it  --privileged --pull=newer --security-opt label=type:unconfined_t     -v $(pwd)/config.toml:/config.toml     -v $(pwd)/output:/output     localhost/bootc-image-builder:tag-validate   --type qcow2 quay.io/centos/centos:stream9
WARNING: The same type, major and minor should not be used for multiple devices.
... elided... ( I don't know enough to know what the warnings mean)
WARNING: The same type, major and minor should not be used for multiple devices.
Generating manifest manifest-qcow2.json
Error: cannot build manifest: image quay.io/centos/centos:stream9 is not a bootc image
2024/06/15 21:02:07 error: cannot build manifest: image quay.io/centos/centos:stream9 is not a bootc image

mvo5

Thank you (and sorry for the somewhat slow reply, we are usually quicker with reviews)!

This looks very nice - I have some suggestions inline about the testing, it includes two potential git commits that could be used here to. Once the testing is sorted this looks good to go to me (but an additonal pair of eyes on this from e.g. @achilleas-k would be nice)

bib/internal/setup/setup_test.go

bib/internal/setup/setup.go

bearrito · 2024-06-19T00:27:51Z

@mvo5 I incorporated your patches. My editor applied my default formatting, not sure there is a formatter the project uses.

I had an issue with test_manifest.py. 5 of the tests were failing due to arch issues. Maybe this is environmental on my end?
Example:

Trying to pull quay.io/centos-bootc/fedora-bootc:eln...
Error: error creating build container: choosing an image from manifest list docker://quay.io/centos-bootc/fedora-bootc:eln: no image found in image index for architecture x86_64, variant "", OS linux
FAILED

================================================================= FAILURES ==================================================================
_______________________________________ test_manifest_disksize[quay.io/centos-bootc/fedora-bootc:eln] __

The specific tag tests were passing though.

sudo -E /home/barrett/.local/bin/pytest -s -v test/test_manifest.py::test_manifest_checks_build_container_is_bootc

=========================================================== slowest 10 durations ============================================================
2.63s call     test/test_manifest.py::test_manifest_checks_build_container_is_bootc[quay.io/centos-bootc/centos-bootc:stream9-False-None]
1.53s call     test/test_manifest.py::test_manifest_checks_build_container_is_bootc[quay.io/centos/centos:stream9-True-image quay.io/centos/centos:stream9 is not a bootc image]
1.43s setup    test/test_manifest.py::test_manifest_checks_build_container_is_bootc[quay.io/centos/centos:stream9-True-image quay.io/centos/centos:stream9 is not a bootc image]
0.00s setup    test/test_manifest.py::test_manifest_checks_build_container_is_bootc[quay.io/centos-bootc/centos-bootc:stream9-False-None]
0.00s teardown test/test_manifest.py::test_manifest_checks_build_container_is_bootc[quay.io/centos-bootc/centos-bootc:stream9-False-None]
0.00s teardown test/test_manifest.py::test_manifest_checks_build_container_is_bootc[quay.io/centos/centos:stream9-True-image quay.io/centos/centos:stream9 is not a bootc image]
============================================================= 2 passed in 5.68s =============================================================

mvo5 · 2024-06-19T07:50:31Z

Thank you for the updated PR!

@mvo5 I incorporated your patches. My editor applied my default formatting, not sure there is a formatter the project uses.

The formatitng changes are something we would like to avoid. The diff should just contain the functional changes. It might be easiest to just "git am" the two patches from #475 (comment) and force push. Note that the "happy" path of ValidateHasContainerTags is tested implicitly by any of the "test_{build,manifest}.py" that test a valid image (e.g. test_manifest..yy::test_manifest_smoke).

I had an issue with test_manifest.py. 5 of the tests were failing due to arch issues. Maybe this is environmental on my end? Example:
Trying to pull quay.io/centos-bootc/fedora-bootc:eln...
Error: error creating build container: choosing an image from manifest list docker://quay.io/centos-bootc/fedora-bootc:eln: no image found in image index for architecture x86_64, variant "", OS linux
FAILED

[..]

Thank you! I will try to reproduce, is there anything unusual about your setup that might help me to reproduce?

achilleas-k · 2024-06-19T10:33:43Z

I got a bit lost reviewing that last commit with all the formatting changes so I edited the commits a bit. Broke out the octal literal format change into its own commit, reverted the formatting changes to test_manifest.py and fixed the import order in a separate commit.

achilleas-k

Thanks for the contribution. This LGTM!

Check that the container has the expected bootc tags early and fail if they are missing.

Use `0o` instead of just `0` for better readability.

Create a mock podman executable script and add it at the top of PATH. Use it to generate output for the container tag validation.

achilleas-k · 2024-07-04T11:05:04Z

Rebased and resolved conflict.

Add new `test_manifest_checks_build_container_is_bootc` test that validates that `botoc-iamge-build manifest` fails with the expected error on a non-bootc image url. Note that this will implicitly check `build` too as the same check is performed in bib for manifest and build (i.e. bib needs to generate a manifest first before it can build). Co-authored-by: bstrausser <bstrausser@locusrobotics.com>

mvo5

Thank you!

mvo5 reviewed Jun 18, 2024

View reviewed changes

bib/internal/setup/setup_test.go Outdated Show resolved Hide resolved

bib/internal/setup/setup.go Outdated Show resolved Hide resolved

mvo5 added the squash-merge label Jun 19, 2024

achilleas-k force-pushed the feature/fail-tag-early branch from f174336 to 74eab7b Compare June 19, 2024 10:31

achilleas-k previously approved these changes Jun 19, 2024

View reviewed changes

achilleas-k removed the squash-merge label Jun 19, 2024

achilleas-k dismissed their stale review via 28a4603 June 19, 2024 14:24

achilleas-k force-pushed the feature/fail-tag-early branch from 74eab7b to 28a4603 Compare June 19, 2024 14:24

achilleas-k previously approved these changes Jun 19, 2024

View reviewed changes

mvo5 enabled auto-merge June 21, 2024 09:20

bstrausser and others added 4 commits July 4, 2024 13:02

bib/internal/setup: validate container tags early

75e9a28

Check that the container has the expected bootc tags early and fail if they are missing.

bib: change octal literal prefix

f4e68f3

Use `0o` instead of just `0` for better readability.

bib/internal/setup: use fake podman binary for testing

8d1546f

Create a mock podman executable script and add it at the top of PATH. Use it to generate output for the container tag validation.

test/test_manifest: fix import order

e78a772

achilleas-k dismissed their stale review via c044c78 July 4, 2024 11:04

achilleas-k force-pushed the feature/fail-tag-early branch from 28a4603 to c044c78 Compare July 4, 2024 11:04

achilleas-k previously approved these changes Jul 4, 2024

View reviewed changes

mvo5 dismissed achilleas-k’s stale review via 3dc4b1e July 4, 2024 14:13

mvo5 force-pushed the feature/fail-tag-early branch from c044c78 to 3dc4b1e Compare July 4, 2024 14:13

mvo5 approved these changes Jul 4, 2024

View reviewed changes

achilleas-k approved these changes Jul 4, 2024

View reviewed changes

mvo5 added this pull request to the merge queue Jul 4, 2024

Merged via the queue into osbuild:main with commit aa6343f Jul 4, 2024
6 of 8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bib/internal/setup: Tag validate early #475

bib/internal/setup: Tag validate early #475

bearrito commented Jun 13, 2024

bearrito commented Jun 15, 2024 •

edited

Loading

mvo5 left a comment

bearrito commented Jun 19, 2024 •

edited

Loading

mvo5 commented Jun 19, 2024

achilleas-k commented Jun 19, 2024

achilleas-k left a comment

achilleas-k commented Jul 4, 2024

mvo5 left a comment

bib/internal/setup: Tag validate early #475

bib/internal/setup: Tag validate early #475

Conversation

bearrito commented Jun 13, 2024

bearrito commented Jun 15, 2024 • edited Loading

mvo5 left a comment

Choose a reason for hiding this comment

bearrito commented Jun 19, 2024 • edited Loading

mvo5 commented Jun 19, 2024

achilleas-k commented Jun 19, 2024

achilleas-k left a comment

Choose a reason for hiding this comment

achilleas-k commented Jul 4, 2024

mvo5 left a comment

Choose a reason for hiding this comment

bearrito commented Jun 15, 2024 •

edited

Loading

bearrito commented Jun 19, 2024 •

edited

Loading