compose: Add --label for build-chunked-oci
#5454
Conversation
|
Skipping CI for Draft Pull Request. |
![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 adds a --label option to the build-chunked-oci command, allowing users to add custom labels to the generated OCI image. The implementation is straightforward and includes a test case to verify the new functionality. My review includes a suggestion to improve the efficiency of argument construction for the labels, making it more consistent with existing code patterns.
| .bootc | ||
| .then_some(["--label", "containers.bootc=1"].as_slice()) | ||
| .unwrap_or_default(); | ||
| let label_args = self.labels.into_iter().map(|v| format!("--label={v}")); |
There was a problem hiding this comment.
This implementation works, but it can be made more efficient and consistent with other argument handling in this file. It allocates a new String for each label, which is unnecessary. Using iter().flat_map(...) avoids these allocations by creating an iterator of string slices (&str), which is more performant and aligns with patterns used elsewhere in this function for constructing command arguments.
| let label_args = self.labels.into_iter().map(|v| format!("--label={v}")); | |
| let label_args = self.labels.iter().flat_map(|v| ["--label", v.as_str()]); |
There was a problem hiding this comment.
That's a good suggestion. I copied this from another entry point that also takes labels but we should switch over both.
|
I didn't test this yet (though CI should sanity-check it). It's an aside that would help something I'm working on but is not necessary. Though I just realized now it could also be used to clean up the FCOS Containerfile at least, where the second phase is full of label-setting (though the |
This is for coreos/fedora-coreos-tracker#1996. I initially wanted to just add another `LABEL` to our Containerfile and hook that up to another build arg, but the problem is that our Containerfile is shared with RHCOS/SCOS and there's no way to make `LABEL` directives conditional. I opened coreos/rpm-ostree#5454 which will fix this but for now to not block on this, let's just slap it on from the cosa side. Once it's folded back into the build process proper, then we can rever this. But the label not existing shouldn't really affect the majority of developers anyway.
This is for coreos/fedora-coreos-tracker#1996. I initially wanted to just add another `LABEL` to our Containerfile and hook that up to another build arg, but the problem is that our Containerfile is shared with RHCOS/SCOS and there's no way to make `LABEL` directives conditional. I opened coreos/rpm-ostree#5454 which will fix this but for now to not block on this, let's just slap it on from the cosa side. Once it's folded back into the build process proper, then we can rever this. But the label not existing shouldn't really affect the majority of developers anyway.
This is trivial to do since the underlying `container-encapsulate` we do already supports this. What becomes really interesting about this though is that in a container build flow where one is using `build-chunked-oci` with the `FROM: ./out.ociarchive` trick, it then becomes possible to _dynamically_ (i.e. from within the build process itself) choose whether to add a label, or determine what value a label should have, which is a common frustration in container image building.
|
Ok yup, works fine! Just rebased to retrigger CI. |
|
@jlebon: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This is trivial to do since the underlying
container-encapsulatewe do already supports this.What becomes really interesting about this though is that in a container build flow where one is using
build-chunked-ociwith theFROM: ./out.ociarchivetrick, it then becomes possible to dynamically (i.e. from within the build process itself) choose whether to add a label, or determine what value a label should have, which is a common frustration in container image building.