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.

Sign up for GitHub

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

Jump to bottom

containers.conf: turn all []string into attributedstring.Slice #1710

Merged
merged 30 commits into from
Oct 26, 2023
Merged

containers.conf: turn all []string into attributedstring.Slice #1710

merged 30 commits into from
Oct 26, 2023

Conversation

vrothberg
Copy link
Member

Please see the individual commits. No docs changes yet as I want to get this wired into Buildah and Podman first.

@vrothberg
Copy link
Member Author

@rhatdan PTAL

@vrothberg
Copy link
Member Author

Buildah PR: containers/buildah#5103

@vrothberg
Copy link
Member Author

Podman PR: containers/podman#20483

Luap99
Luap99 reviewed Oct 25, 2023
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Just some small comments

pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated
@@ -1143,7 +1143,7 @@ func (c *Config) FindHelperBinary(name string, searchPATH bool) (string, error)
return exec.LookPath(name)
}
configHint := "To resolve this error, set the helper_binaries_dir key in the `[engine]` section of containers.conf to the directory containing your helper binaries."
if len(c.Engine.HelperBinariesDir) == 0 {
if len(c.Engine.HelperBinariesDir.Values) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this uses Values vs Get()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Get() will never return nil, so it adds some cost.

Copy link
Member

Choose a reason for hiding this comment

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

Why did you added this special handling to Get()? To me this seems to add unnecessary complexity. Returning nil shouldn't hurt or do podman/buildah tests fail becauase of it?

Copy link
Member Author

Choose a reason for hiding this comment

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

buildah tests failed. I can follow up on that if you want to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Although it may be safer to let Get() return a real copy in the future.

Copy link
Member

Choose a reason for hiding this comment

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

I am fine with it but as you said it does extra allocations which means worse performance and memory footprint. It won't be noticeable (I assume) so it is not worth to block/argue about it but many small things start adding up at some point. Copying would definitely make it more expensive with the advantage that the API will be much safer with it.
The main issue is that technically callers can bypass that and just use Values directly so it is impossible to provide a safe API. Ideally go would allow us to keep the fields private but that won't work with the toml Unmarshaller obviously.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had the very same line of thoughts. To really avoid callers abusing containers.conf, the loading must be made private and then we can add a carefully curated API (as we do in registries.conf).

pkg/config/config_local.go Outdated Show resolved Hide resolved
pkg/config/config_local.go Outdated Show resolved Hide resolved
pkg/config/default.go Outdated Show resolved Hide resolved
@vrothberg
attributedstring.Slice: add Set() method
137c400 
Needed in Buildah (and potentially Podman later on) where some options
must be overridden.  Ultimately this should be avoided whenever possible
but this is not my goal at the present.

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
attributedstring: add NewSlice func
c3c1aec 
Convenience function for callers.

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn devices into attributedstring.Slice
a71ef23 
The tests are messy and should be turned into table-driven tests but I
do not have time at the moment.

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn Annotations into attributedstring.Slice
b7a5a13 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn CgroupConf into attributedstring.Slice
7a6915b 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn Capabilities into attributedstring.Slice
3a0009f 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn DefaultSysctls into attributedstring.Slice
fc58fb6 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn Ulimites into attributedstring.Slice
0c3b731 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn DNSServers into attributedstring.Slice
56d438f 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn DNSOptions into attributedstring.Slice
53cc626 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn DNSSearches into attributedstring.Slice
7b8924f 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn ConmonEnvVars into attributedstring.Slice
2fbb01d 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn ConmonPath into attributedstring.Slice
7506733 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn ConmonRsPath into attributedstring.Slice
915d4e1 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn ComposeProviders into attributedstring.Slice
dc9dc83 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn engine Env into attributedstring.Slice
d4cdf16 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn HelperBinariesDir into attributedstring.Slice
edac755 
Also tag it as omitempty to fix the test.

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn DefaultHooksDir into attributedstring.Slice
a9b4b08 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn NetworkCmdOptions into attributedstring.Slice
b354c1d 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn AddCompression into attributedstring.Slice
f22b5d8 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn RuntimePath into attributedstring.Slice
f51fdbe 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn RuntimeSupportsJSON into attributedstring.Slice
1ca85fe 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn RuntimeSupportsNoCgroups into attributedstring.Slice
cc5a288 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn RuntimeSupportsKVM into attributedstring.Slice
e34516e 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn CNIPluginDirs into attributedstring.Slice
fd0b58b 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn NetavarkPluginDirs into attributedstring.Slice
ee5721d 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn PastaOptions into attributedstring.Slice
4aaa7e9 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: turn machine Volumes into attributedstring.Slice
6fed91f 
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: fix remote tests
f335c36 
They broke after the attributedstring.Slice migrations.

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
pkg/config: use NewSlice, Get and Set where possible
82e88cc 
To improve the code and reduce memory allocations.

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@vrothberg
Copy link
Member Author

Updated, see last commit. Used NewSlice, Get and Set where possible.

@vrothberg vrothberg marked this pull request as ready for review October 26, 2023 08:33
Luap99
Luap99 approved these changes Oct 26, 2023
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 26, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, vrothberg

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vrothberg
Copy link
Member Author

@rhatdan PTAL

@rhatdan
Copy link
Member

rhatdan commented Oct 26, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 26, 2023
@openshift-ci openshift-ci bot merged commit 78e0a90 into containers:main Oct 26, 2023
7 checks passed
@vrothberg vrothberg deleted the RUN-1934 branch October 26, 2023 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Luap99 Luap99 Luap99 approved these changes

Assignees

@rhatdan rhatdan

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vrothberg @rhatdan @Luap99