Skip to content

Conversation

cgwalters
Copy link
Collaborator

No description provided.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a significant refactoring of the storage handling logic. The new Storage struct in crates/lib/src/store now acts as a central point for accessing different storage backends like ostree and the container image store, improving encapsulation by removing the Deref implementation and introducing explicit accessors. The imgstorage module has been renamed to podstorage and its Storage struct to CStorage for clarity. These changes have been propagated consistently across the codebase. The review identified one minor issue with redundant code. Overall, this is a good refactoring that improves the code structure.

To make clearer what it is vs the other two storage types.

Signed-off-by: Colin Walters <[email protected]>
The goal here is to make all three types of storage effectively
equal peers.

Signed-off-by: Colin Walters <[email protected]>
This was a copy-pasteo from the ostree bits.

Signed-off-by: Colin Walters <[email protected]>
It's just way too confusing we have two structs called `Storage`.
I'm going to rename one of them after this. But as prep for that,
switch to dynamically initialzing the containers-storage version
from the main Storage.

Signed-off-by: Colin Walters <[email protected]>
Previously we had two different `Storage` structs, this
ensures that the main one is primary and `CStorage` is secondary.

This should be less confusing.

Signed-off-by: Colin Walters <[email protected]>
Naming things is hard, but as of right now since this storage
instance doesn't hold the booted host, let's call it
`podstorage` to make clear is association and purpose in
being used by podman and related tools.

Signed-off-by: Colin Walters <[email protected]>
@Johan-Liebert1
Copy link
Collaborator

So, if I understand correctly, we want Store as an abstraction over all different storage we have and we want to have a single function call instead of if {} else {} everywhere? Looks okay to me, just one question above

@cgwalters
Copy link
Collaborator Author

So, if I understand correctly, we want Store as an abstraction over all different storage we have and we want to have a single function call instead of if {} else {} everywhere? Looks okay to me, just one question above

Store holds references to the different storages, but it isn't itself an abstraction yet.

That said, the more I think about things the more I feel what will make sense is to eventually aim for truly "unified storage" where we always pull bootable host containers into the containers-storage instance, and then copy from there into composefs or so. It'd give us a decent mix of the advantages of both.

@Johan-Liebert1 Johan-Liebert1 merged commit 147f16c into bootc-dev:main Aug 19, 2025
21 of 27 checks passed
Johan-Liebert1 added a commit to Johan-Liebert1/bootc that referenced this pull request Aug 19, 2025
Update according to API changes introduced in
bootc-dev#1525

Signed-off-by: Johan-Liebert1 <[email protected]>
Johan-Liebert1 added a commit to Johan-Liebert1/bootc that referenced this pull request Aug 19, 2025
Update according to API changes introduced in
bootc-dev#1525

Signed-off-by: Johan-Liebert1 <[email protected]>
p5 pushed a commit to rsturla/bootc that referenced this pull request Aug 22, 2025
Update according to API changes introduced in
bootc-dev#1525

Signed-off-by: Johan-Liebert1 <[email protected]>
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.

2 participants