containers / image

Work with containers' images
Apache License 2.0
842 stars 365 forks source link

Read registries.conf from XDG_CONFIG_HOME if set #2450

Open martinwe-adfinis opened 2 weeks ago

martinwe-adfinis commented 2 weeks ago

Addresses #1647.

martinwe-adfinis commented 2 weeks ago

The tests have not been adapted yet, so they will fail if $XDG_CONFIG_HOME is set to a non-default path. The workaround is currently (unset XDG_CONFIG_HOME; go test).

If anyone knows how to extend/adapt the tests, contributions welcome. :sweat_smile:

martinwe-adfinis commented 2 weeks ago

I just stumbled over #1011 and the feedback there to use homedir.GetConfigHome(). That seems cleaner, I Will adapt this.

--edit For some reason that makes this one check fail:

--- FAIL: TestNewConfigWrapper (0.00s)
    system_registries_v2_test.go:298: 
            Error Trace:    /home/martinwe/devel/github.com/containers/image/pkg/sysregistriesv2/system_registries_v2_test.go:298
            Error:          Not equal: 
                            expected: "/tmp/TestNewConfigWrapper3162342516/001/.config/containers/registries.conf"
                            actual  : "/etc/containers/registries.conf"

                            Diff:
                            --- Expected
                            +++ Actual
                            @@ -1 +1 @@
                            -/tmp/TestNewConfigWrapper3162342516/001/.config/containers/registries.conf
                            +/etc/containers/registries.conf
            Test:           TestNewConfigWrapper
FAIL
exit status 1
FAIL    github.com/containers/image/v5/pkg/sysregistriesv2  0.013s

For reference, the diff to the current version (where everything passes):

diff --git a/pkg/sysregistriesv2/system_registries_v2.go b/pkg/sysregistriesv2/system_registries_v2.go
index 5dbf5e6e..cb0340b6 100644
--- a/pkg/sysregistriesv2/system_registries_v2.go
+++ b/pkg/sysregistriesv2/system_registries_v2.go
@@ -564,10 +564,9 @@ func newConfigWrapper(ctx *types.SystemContext) configWrapper {
 // it exists only to allow testing it with an artificial home directory.
 func newConfigWrapperWithHomeDir(ctx *types.SystemContext, homeDir string) configWrapper {
        var wrapper configWrapper
-       var configHome string
-       var envVarFound bool

-       if configHome, envVarFound = os.LookupEnv("XDG_CONFIG_HOME"); !envVarFound {
+       configHome, err := homedir.GetConfigHome()
+       if err != nil {
                configHome = filepath.Join(homeDir, ".config")
        }
        userRegistriesFilePath := filepath.Join(configHome, userRegistriesFile)

I suspects it now fails to handle /tmp/TestNewConfigWrapper3162342516/001/.config/containers/registries.conf as an existing user config file and therefore falls back to the system-wide config, but I don't see why. The existence checks hasn't been touched.

mtrmac commented 1 week ago

Thanks!

The tests have not been adapted yet, so they will fail if $XDG_CONFIG_HOME is set to a non-default path.

That needs to be handled in the tests; ideally the tests should cover both the case when the variable is set, and when it is not.

See how other tests are calling t.Setenv / os.Unsetenv. Alternatively, it would be possible to parametrize the function similarly to how newConfigWrapperWithHomeDir exists to parametrize the home directory, but that tests a bit less of the code.