config: don't call homedir on init()

[thaJeztah]

The homedir.Get() function can call out to external applications to get user information.

This patch changes the package to lazily obtain the user's home-directory on first use, instead of when initializing the package.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

[thaJeztah]

@silvin-lubecki @simonferquel @tiborvass PTAL

[kolyshkin]

The homedir.Get() function can call out to external applications to get user information.

Can you elaborate on this?

My $0.02: since golang 1.8 or so 1.9 user.Current() result is cached. Source: golang/go@2ca5d10

[thaJeztah]

Can you elaborate on this?

arg; you're right; I was thinking the package was still using the runc / libcontainer stuff, but that was removed in moby/moby@a8608b5#diff-1b2f13bed97df1999bf7f922eb2f0909

[silvin-lubecki]

LGTM

[thaJeztah]

@kolyshkin let me know what you think; I think it still makes sense to get this info lazily, but happy to close otherwise

[thaJeztah]

Actually; let me make one small change, and don't attempt to set the dir if it's non-empty (which could be if a user of this package calls SetDir() before Dir()

[kolyshkin]

Actually; let me make one small change, and don't attempt to set the dir if it's non-empty (which could be if a user of this package calls SetDir() before Dir()

I don't like this change -- now you have two "run once" guards, and they are scattered.

Maybe it's better to rewrite Dir() to encapsulate setConfigDir(), i.e. the same way that user.Current is written (except there's no need to do a copy a string being returned)?

https://github.com/golang/go/blob/46e0d724b3f14fd0d350123bbf101e815b493791/src/os/user/lookup.go#L9-L28

[thaJeztah]

@kolyshkin it's a bit problematic; tbh I think this package is way too complex

Dir() (and thus configDir) is quite ambiguous; it's neither the "default" location, nor the location of the current config.

It's a global / package level variable that can be set to a default, but also overwritten by a "setter".

eg, someone could set the directory;

cli/cli/config/config.go

Line 46 in 7a279af

func SetDir(dir string) {

In which case we don't want to overwrite it with the default

Someone could load the config, and specify the configDir

cli/cli/config/config.go

Lines 82 to 85 in 7a279af

	func Load(configDir string) (*configfile.ConfigFile, error) {
	if configDir == "" {
	configDir = Dir()
	}

In which case Dir() is not changed, and thus doesn't reflect the current config's location. But if no path is specified, it is used as the default, and thus the config's location

And worst case; if the specified location couldn't be found, in which case the current config location is neither the one specified, nor the default one, but a completely different path:

cli/cli/config/config.go

Lines 108 to 112 in 7a279af

	// Can't find latest config file so check for the old one
	homedir, err := os.UserHomeDir()
	if err != nil {
	return configFile, errors.Wrap(err, oldConfigfile)
	}

[jonjohnsonjr]

This tripped us up, so I had to land a workaround: google/go-containerregistry#568

I'm in favor of this change :)

[thaJeztah]

@kolyshkin PTAL

[cpuguy83]

LGTM