container: Add path-based compute-composefs-digest command #1863
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new bootc container compute-composefs-digest command to compute the digest from a filesystem path, which is a useful addition for containerized environments. The existing functionality is preserved under a new name, compute-composefs-digest-from-storage, and the related script is updated accordingly. The changes are well-structured and the implementation of the new command is solid.
My main feedback is regarding a piece of duplicated code for setting up a temporary ComposefsRepository. I've left a comment with a suggestion to refactor this into a helper function to improve maintainability. Overall, this is a good change.
|
Worth noting explicitly that these two approaches do not agree, which is mostly (or entirely?) due to containers/composefs-rs#132 and all that mess. But... progress. |
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Of all of this stuff I think tests of some form are needed pre-merge, let's all try to hold ourselves to that standard.
jeckersb
force-pushed
the
compute-composefs-in-target
branch
from
b2418b6 to
6fa5bc3
Compare
Add a new `bootc container compute-composefs-digest` command that computes the bootable composefs digest directly from a filesystem directory path, defaulting to `/target`. This enables computing digests in container environments without requiring access to container storage or a booted host system. The existing container-storage-based behavior is preserved and renamed to `compute-composefs-digest-from-storage` (hidden). The `hack/compute-composefs-digest` script is updated to use the renamed command. The core digest computation logic is extracted into a new `bootc_composefs::digest` module with: - `new_temp_composefs_repo()` helper for DRY temp repository creation - `compute_composefs_digest()` function with "/" path rejection Unit tests and an integration test verify the command works correctly, producing valid SHA-512 hex digests with consistent results across multiple invocations. Exact digest values are not asserted due to environmental variations (SELinux labels, timestamps, etc.). Closes: bootc-dev#1862 Assisted-by: OpenCode (Claude Opus 4.5) Signed-off-by: John Eckersberg <jeckersb@redhat.com>
jeckersb
force-pushed
the
compute-composefs-in-target
branch
from
6fa5bc3 to
d05be98
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new path-based compute-composefs-digest command, which is a valuable addition for containerized environments. The existing storage-based functionality has been cleanly refactored into a new hidden command, compute-composefs-digest-from-storage. The implementation is well-structured, with the core logic neatly encapsulated in the new digest.rs module, complete with its own unit tests. An integration test is also included to validate the new command. Overall, the changes are logical and well-executed. I have one suggestion to improve maintainability by reducing code duplication in the tests.
| // Create directories required by transform_for_boot | ||
| fs::create_dir_all(root.join("boot"))?; | ||
| fs::create_dir_all(root.join("sysroot"))?; | ||
|
|
||
| // Create usr/bin/hello (executable) | ||
| let usr_bin = root.join("usr/bin"); | ||
| fs::create_dir_all(&usr_bin)?; | ||
| let hello_path = usr_bin.join("hello"); | ||
| fs::write(&hello_path, "test\n")?; | ||
| fs::set_permissions(&hello_path, fs::Permissions::from_mode(0o755))?; | ||
|
|
||
| // Create etc/config (regular file) | ||
| let etc = root.join("etc"); | ||
| fs::create_dir_all(&etc)?; | ||
| let config_path = etc.join("config"); | ||
| fs::write(&config_path, "test\n")?; | ||
| fs::set_permissions(&config_path, fs::Permissions::from_mode(0o644))?; |
There was a problem hiding this comment.
The logic to create a test filesystem here is a duplicate of the create_test_filesystem helper function in crates/lib/src/bootc_composefs/digest.rs. To improve maintainability and avoid having to update two places for any future changes to the test filesystem structure, consider extracting this logic into a shared test utility. Since bootc and tests-integration are separate crates, this might involve creating a small, shared test-only crate or exposing the helper from bootc under a test-util feature flag.
There was a problem hiding this comment.
Per discussion I'd actually like to adapt the fixture.rs code from the ostree side - another option though is composefs dumpfile format.
| use std::os::unix::fs::PermissionsExt; | ||
|
|
||
| // Create temp directory with test filesystem structure | ||
| let td = tempfile::tempdir()?; |
There was a problem hiding this comment.
This is fine as is but I think cap-std is a great choice for the test use cases. It's really easy to accidentally forget to prepend the rootdir and then suddenly your unit tests are writing to the current working directory etc.
|
We have this as (edit I mean as a followup) |
2 participants
Add a new
bootc container compute-composefs-digestcommand that computesthe bootable composefs digest directly from a filesystem directory path,
defaulting to
/target. This enables computing digests in containerenvironments without requiring access to container storage or a booted
host system.
The existing container-storage-based behavior is preserved and renamed
to
compute-composefs-digest-from-storage(hidden). Thehack/compute-composefs-digestscript is updated to use the renamed command.
Computing the digest on
/is explicitly rejected with a clear errormessage, as this is not supported.
Closes: #1862
Assisted-by: OpenCode (Claude Opus 4.5)
Signed-off-by: John Eckersberg jeckersb@redhat.com