Remove containers/common/pkg/config from pkg/util

[rhatdan]

Does this PR introduce a user-facing change?

None

[rhatdan]

@mtrmac is this what you are thinking?

[mtrmac]

@mtrmac is this what you are thinking?

Something vaguely along these lines.

AFAICS this doesn’t work yet because …/podman/v5/util is also included by vendor/github.com/containers/podman/v5/pkg/bindings/containers/create.go , per

package main

import (
	_ "github.com/containers/podman/v5/pkg/bindings"
	_ "github.com/containers/podman/v5/pkg/bindings/containers"
	_ "github.com/containers/podman/v5/pkg/bindings/generate"
	_ "github.com/containers/podman/v5/pkg/bindings/images"
	_ "github.com/containers/podman/v5/pkg/bindings/kube"
	_ "github.com/containers/podman/v5/pkg/bindings/manifests"
	_ "github.com/containers/podman/v5/pkg/bindings/network"
	_ "github.com/containers/podman/v5/pkg/bindings/play"
	_ "github.com/containers/podman/v5/pkg/bindings/pods"
	_ "github.com/containers/podman/v5/pkg/bindings/secrets"
	_ "github.com/containers/podman/v5/pkg/bindings/system"
	_ "github.com/containers/podman/v5/pkg/bindings/volumes"
)

func main() {

}

in a new directory + go mod tidy && go mod vendor and inspecting the result.

I didn’t check further how deep the rabbit hole goes; it might involve several more package splits. Or it might not.

---

We got here by having a jack-of-all-trades pkg/util package; I don’t think moving the code into a different utils package is structurally an improvement. Move that into a separate internal/config (or maybe pkg/config? I think internal would be cleaner but it’s also not widely used).

---

That’s clearly not this PR, but the Podman codebase has 29 other calls to config.Default(). If we have an init that exists on failure, it seems to me those 29 calls could be migrated to DefaultContainerConfig and we could remove some handling of unreachable errors.

… or maybe not? config.Reload exists… and if I am skimming the code correctly, that affects config.Default() but not DefaultContainerConfig(). I don’t understand what is going on. Maybe DefaultContainerConfig should not exist at all?

[Luap99]

… or maybe not? config.Reload exists… and if I am skimming the code correctly, that affects config.Default() but not DefaultContainerConfig(). I don’t understand what is going on. Maybe DefaultContainerConfig should not exist at all?

Agreed, at least in the cmd/podman dirs one should use registry.PodmanConfig().ContainersConfDefaultsRO instead

For the other callers I would simply call config.Default() and handle the error there

[Luap99]

If the goal is to completely get rid of c/common/pkg/config from pkg/bindings it is much more work

generated with goda graph "remote=1(reach(./pkg/bindings/...:all, github.com/containers/common/pkg/config))" | dot -Tsvg -o graph.svg assuming this tool is correct but the result looks fine to me on quick look

[packit-as-a-service]

Ephemeral COPR build failed. @containers/packit-build please check.

[Luap99]

please use registry.PodmanConfig().ContainersConfDefaultsRO in all cmd/podman/... files instead

[Luap99]

I really think this is just a miss feature abusing init(). Of course it is a pre existing issue but this doesn't feel right at all. For starters exit code 1 is wrong as we document 125 as default exit code. But in general storing a copy of the config is just wrong.

So please just convert all the callers to config.Default() and handle the errors there. As @mtrmac pointed this here does not work with config.Reload()

[Luap99]

LGTM

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

• OWNERS [Luap99,rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mtrmac]

ACK, this is clearly an improvement.

---

The test failure looks rather worrisome:

$ podman [options] stop --all -t 0
SIGABRT: abort
PC=0x40cd0e m=0 sigcode=0

with nothing obvious in the backtrace.

[Luap99]

That is a known flake: #23685

[baude]

/lgtm

thanks @rhatdan