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.

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

Read registries.conf from XDG_CONFIG_HOME if set #2450

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

martinwe-adfinis
Copy link

Addresses #1647.

@martinwe-adfinis
Copy link
Author

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. 😅

martinwe-adfinis added a commit to martinwe-adfinis/localdir that referenced this pull request Jun 13, 2024
Also set CONTAINERS_REGISTRIES_CONF, as it appears to hardcode ~/.config
for XDG_CONFIG_HOME [1] [2].

[1] containers/image#1647
[2] containers/image#2450
@martinwe-adfinis
Copy link
Author

martinwe-adfinis commented Jun 13, 2024

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
Copy link
Collaborator

mtrmac commented Jun 20, 2024

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.

@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Jun 20, 2024
@rhatdan
Copy link
Member

rhatdan commented Aug 7, 2024

@martinwe-adfinis still working on this?

@martinwe-adfinis
Copy link
Author

@rhatdan Not yet, unfortunately. I also cannot give an ETA when I will find some time again to look at the tests or complete the issue pointed out in my comment above. Apologies for the delay.

Feel free to have a look at if you want.

martinwe-adfinis added a commit to martinwe-adfinis/localdir that referenced this pull request Sep 1, 2024
Also set CONTAINERS_REGISTRIES_CONF, as it appears to hardcode ~/.config
for XDG_CONFIG_HOME [1] [2].

[1] containers/image#1647
[2] containers/image#2450
martinwe-adfinis added a commit to martinwe-adfinis/localdir that referenced this pull request Sep 1, 2024
Also set CONTAINERS_REGISTRIES_CONF, as it appears to hardcode ~/.config
for XDG_CONFIG_HOME [1] [2].

[1] containers/image#1647
[2] containers/image#2450
martinwe-adfinis added a commit to martinwe-adfinis/localdir that referenced this pull request Sep 1, 2024
Also set CONTAINERS_REGISTRIES_CONF, as it appears to hardcode ~/.config
for XDG_CONFIG_HOME [1] [2].

[1] containers/image#1647
[2] containers/image#2450
martinwe-adfinis added a commit to martinwe-adfinis/localdir that referenced this pull request Sep 1, 2024
Also set CONTAINERS_REGISTRIES_CONF, as it appears to hardcode ~/.config
for XDG_CONFIG_HOME [1] [2].

[1] containers/image#1647
[2] containers/image#2450
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature A request for, or a PR adding, new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants