manifest: addCompression use default from containers.conf
#5014
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
flouthoc
force-pushed
the
default-add-compression
branch
from
06cc75b
to
8f6f843
Compare
|
PR similar to containers/podman#19790 |
No issues, I just created PR for consistency with podman can drop PR if everybody agrees with it |
|
Where we can, I would want buildah to work with containers.conf? |
Signed-off-by: Aditya R <arajan@redhat.com>
flouthoc
force-pushed
the
default-add-compression
branch
from
8f6f843
to
8d0544b
Compare
|
Rebased in-case if consensus is made to merge this one. |
There was a problem hiding this comment.
I'm torn. Generally, I am supportive of supporting containers.conf in Buildah but as mentioned before, I do not think it's a nice user experience if only 1 option is used. What about compression_format? I think we should at a minimum support all fields which are sensitive to compression.
Then we can tackle the rest. But imagine being a user trying to figure out what works and what doesn't. It's even hard for maintainer as we had to look at the code.
|
I'm fence-sitting too. I'm thinking primarily of a Buildah Only user. Hopping into containers.conf, seeing the myriad of options, and only being able to use one. But I see the other side of the coin, too, where you want the consistency to happen between Buildah and Podman. All in all, I'm slightly leaning toward including containers.conf, but we'll need a lot of Buildah doc in and out of it explaining what is and is not used by Buildah. |
|
So, I am OK to go step-by-step and containers.conf support gradually BUT - as mentioned above - we should at least support the other compression options as well. |
|
Agreed |
| @@ -49,6 +50,11 @@ type manifestInspectOpts = struct { | |||
| } | |||
|
|
|||
| func init() { | |||
| containerConfig, err := config.Default() | |||
| if err != nil { | |||
| logrus.Errorf(err.Error()) | |||
There was a problem hiding this comment.
Always use pass a format specifier as the first argument for functions like Errorf(), so that we don't have to track down surprises later. I am concerned about parsing this again in this init(), when we're doing the same thing in both from.go and main.go.
| cat > $contextdir/containers.conf << _EOF | ||
| [engine] | ||
| add_compression = ["zstd"] | ||
| _EOF |
There was a problem hiding this comment.
These can be regular files in the source tree, since their content isn't dynamic.
|
@flouthoc any update coming on this one? |
|
A friendly reminder that this PR had no activity for 30 days. |
|
@flouthoc friendly ping. |
|
A friendly reminder that this PR had no activity for 30 days. |
Replaces: containers#5014 Signed-off-by: Aditya R <arajan@redhat.com> Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Replaces: containers#5014 Signed-off-by: Aditya R <arajan@redhat.com> Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Replaces: containers#5014 Signed-off-by: Aditya R <arajan@redhat.com> Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Replaces: containers#5014 Signed-off-by: Aditya R <arajan@redhat.com> Signed-off-by: Daniel J Walsh <dwalsh@redhat.com> Signed-off-by: tomsweeneyredhat <tsweeney@redhat.com>
Replaces: containers#5014 Signed-off-by: Aditya R <arajan@redhat.com> Signed-off-by: Daniel J Walsh <dwalsh@redhat.com> Signed-off-by: tomsweeneyredhat <tsweeney@redhat.com>
Replaces: containers#5014 Signed-off-by: Aditya R <arajan@redhat.com> Signed-off-by: Daniel J Walsh <dwalsh@redhat.com> Signed-off-by: tomsweeneyredhat <tsweeney@redhat.com>
What type of PR is this?
What this PR does / why we need it:
How to verify it
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?