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

Status tweaks #798

Merged
merged 3 commits into from
Sep 24, 2024
Merged

Status tweaks #798

merged 3 commits into from
Sep 24, 2024

Conversation

cgwalters
Copy link
Collaborator

status: Change transport display

Registry is the assumed default; let's be quieter (less output)
in this default/expected case.

If we're using something else (e.g. oci) then put it
inline with the image name as that's something one can
pass to e.g. skopeo.

Signed-off-by: Colin Walters [email protected]


status: Minor code refactorings

  • Rename variable to slot_name as I think it's a bit clearer

Signed-off-by: Colin Walters [email protected]


status: Factor out helpers for rendering container image

Prep for more work; no functional changes.

Signed-off-by: Colin Walters [email protected]


Registry is the assumed default; let's be quieter (less output)
in this default/expected case.

If we're using something else (e.g. `oci`) then put it
inline with the image name as that's something one can
pass to e.g. `skopeo`.

Signed-off-by: Colin Walters <[email protected]>
- Rename variable to `slot_name` as I think it's a bit clearer

Signed-off-by: Colin Walters <[email protected]>
Prep for more work; no functional changes.

Signed-off-by: Colin Walters <[email protected]>
@djach7
Copy link
Contributor

djach7 commented Sep 24, 2024

Just tried to test this locally for a bit and kept running into this error:

ERROR Status: Computing status: Booted deployment: Reading deployment metadata: Missing base image ref ostree/container/blob/d5f71a3300fe94bed5ec6cbf5a6e508a0da96d48057ba76d80a8426add6ef9ee

Any ideas what's causing that? I don't think I'm setting up the bootc environment wrong, as other commands (like bootc help and bootc --version) work fine

@cgwalters
Copy link
Collaborator Author

see #800

@djach7
Copy link
Contributor

djach7 commented Sep 24, 2024

Ah, I see. I'll review as much as I can then without being able to test the full thing. My apologies for not staying more on top of my github notifications

@cgwalters
Copy link
Collaborator Author

#800 only bites for the upgrade case, which is how it sailed through our primary CI. If you create a fresh image with current bootc git it will work. So e.g. podman-bootc run <image> etc.

Copy link
Contributor

@djach7 djach7 left a comment

Choose a reason for hiding this comment

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

LGTM, I need to read about Cow and I left a non-blocking question but it's a certainly a cleaner implementation overall. Agreed that slot_name is a better var name.

Will double check again locally once #800 is resolved, but I'm ok with merging this before that.

Cow::Borrowed(imagename)
} else {
// But for non-registry we include the transport
Cow::Owned(format!("{transport}:{imagename}"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I need to read about how Cow works but I like this implementation. It does make a lot more sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cow basically abstracts over borrowed/owned data here, it's honestly a pointless micro-optimization but it's not hard to do either.

Ok(())
}

fn human_render_ostree(mut out: impl Write, slot_name: &str, _ostree_commit: &str) -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this where we'd implement the documentation for additional deployments like rpm-ostree status?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep

@cgwalters cgwalters merged commit c5f80c9 into containers:main Sep 24, 2024
31 of 34 checks 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.

2 participants