APP-214 Load Client info in getter function #2095

aiordache · 2019-09-20T18:03:08Z

Change required by PR-625

- What I did
Moved cli config load in ClientInfo() function such that when calling it we don't have to initialize the struct in case it isn't.
- How I did it
Load cli config on the first call of ClientInfo().
- How to verify it
ClientInfo() should always return an object with the config data already loaded.
- Description for the changelog
Load cli config on the first call to the ClientInfo() from the cli interface
- A picture of a cute animal (not mandatory but encouraged)

silvin-lubecki

@aiordache can you rebase on top of master?

silvin-lubecki · 2019-09-27T16:00:13Z

cli/command/cli.go

+	if experimentalValue = os.Getenv("DOCKER_CLI_EXPERIMENTAL"); experimentalValue == "" {
+		experimentalValue = cli.ConfigFile().Experimental
+	}
+	hasExperimental, _ := isEnabled(experimentalValue)


Here we are dropping the error, so we will be silent if the experimentalValue is not valid. I don't want to panic here, but returning an error in ClientInfo() would break many people. @thaJeztah any idea?

Looking at it if we can find a way around that. The problem is that with these changes, these "getters" are no longer "getters"; they're doing way more than that (a getter should never return an error, but now they're loading, validating, and returning the info

thaJeztah

Looking at some options, and have pushes a branch with a possible approach

thaJeztah · 2019-10-04T09:17:56Z

cli/command/cli.go

+	if cli.configFile != nil {
+		return cli.configFile
+	}
+	cli.configFile = cliconfig.LoadDefaultConfigFile(cli.err)


Small nit; I'm usually in favor of doing an "early return", but given that this function is really small, perhaps this would be slightly cleaner;

// ConfigFile returns the ConfigFile func (cli *DockerCli) ConfigFile() *configfile.ConfigFile { if cli.configFile == nil { cli.configFile = cliconfig.LoadDefaultConfigFile(cli.err) } return cli.configFile }

We should update the Description as well, to mention that it also loads the configuration file.

Something like:

// ConfigFile loads the ConfigFile if it has not yet been loaded, and returns it

Actually, looking at this; I wonder if this loads the correct config-file, because it's not taking the --config flag into account? 🤔

cli/cli/command/cli.go

Lines 202 to 210 in 0f337f1

if opts.ConfigDir != "" {

cliconfig.SetDir(opts.ConfigDir)

}

if opts.Common.Debug {

debug.Enable()

}

cli.configFile = cliconfig.LoadDefaultConfigFile(cli.err)

thaJeztah · 2019-10-04T09:44:07Z

cli/command/cli.go

+	if experimentalValue = os.Getenv("DOCKER_CLI_EXPERIMENTAL"); experimentalValue == "" {
+		experimentalValue = cli.ConfigFile().Experimental
+	}
+	hasExperimental, _ := isEnabled(experimentalValue)


Looking at it if we can find a way around that. The problem is that with these changes, these "getters" are no longer "getters"; they're doing way more than that (a getter should never return an error, but now they're loading, validating, and returning the info

thaJeztah · 2019-10-04T12:16:00Z

cli/command/cli.go

+	}
+	hasExperimental, _ := isEnabled(experimentalValue)
+
+	version := ""


this clashes with the github.com/docker/cli/cli/version import

Suggested change

version := ""

v := ""

thaJeztah · 2019-10-04T12:25:17Z

cli/command/cli.go

+	if cli.client != nil {
+		version = cli.client.ClientVersion()
+	} else {
+		version = api.DefaultVersion


Is this also a problem if this is called from the plugin? (as in; if there's no client, it will always return the default version, which may not match with the daemon that is used

thaJeztah · 2019-10-04T12:54:43Z

OK, pushed one possible alternative, but it's also not perfect; https://github.com/docker/cli/compare/master...thaJeztah:app-214_client_info_load_func_alt?expand=1

Description (and change) is in this commit dd73b46;

Possible approach for client info

split ClientInfo() into ClientInfo() and loadClientInfo()
split ConfigFile() into ConfigFile() and loadConfigFile()
ConfigFile() and ClientInfo() call their corresponding loadXX function
if it has not yet been loaded; this allows them to be used before
Initialize() was called.
Initialize() always (re-)loads the configuration; this makes sure
that the correct configuration is used when actually calling commands.

I think the problem remains that (IIUC), the client is not initialised at that point, so we're still working with partial / incomplete data (config-dir may be incorrect, and api-client not initialised, so we don't know the API version that the daemon supports, or if the daemon has experimental features enabled).

I also explored the "functional arguments" variant for a bit, but I wasn't fully happy with that yet, because they would also have the same problem, and have to be used in the correct order (load flags, load config, load client, get clientinfo). I think it looks cleaner with those options, but that needs more work (and thinking); https://github.com/docker/cli/compare/master..thaJeztah:more_options?expand=1

GordonTheTurtle · 2019-11-07T09:01:55Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "app-214_client_info_load_func" git@github.com:aiordache/cli.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354319888
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

cli/command/cli.go

silvin-lubecki

LGTM

Signed-off-by: Anca Iordache <anca.iordache@docker.com> Possible approach for client info - split ClientInfo() into ClientInfo() and loadClientInfo() - split ConfigFile() into ConfigFile() and loadConfigFile() - ConfigFile() and ClientInfo() call their corresponding loadXX function if it has not yet been loaded; this allows them to be used before Initialize() was called. - Initialize() *always* (re-)loads the configuration; this makes sure that the correct configuration is used when actually calling commands. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Once app is GA and no longer hidden experimental we will want to hide the cnab-bundle-json flag if the user didn't activate experimental features. docker/cli was updated to latest that had the fix for cli plugins to be able to get the client configuration (docker/cli#2095)

Once app is GA and no longer hidden experimental we will want to hide the cnab-bundle-json flag if the user didn't activate experimental features. docker/cli was updated to latest that had the fix for cli plugins to be able to get the client configuration (docker/cli#2095) Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>

aiordache requested review from albers, silvin-lubecki and thaJeztah as code owners September 20, 2019 18:03

GordonTheTurtle added the status/0-triage label Sep 20, 2019

aiordache mentioned this pull request Sep 20, 2019

[WIP] APP-214 Enable 'docker app validate' for experimental mode only docker-archive-public/docker.app#625

Merged

aiordache force-pushed the app-214_client_info_load_func branch from f82b509 to 577cf52 Compare September 23, 2019 08:29

albers removed their request for review September 26, 2019 08:37

silvin-lubecki reviewed Sep 27, 2019

View reviewed changes

aiordache force-pushed the app-214_client_info_load_func branch from 577cf52 to 7091f8e Compare October 3, 2019 10:12

aiordache changed the title ~~Load Client info in getter function~~ APP-214 Load Client info in getter function Oct 3, 2019

thaJeztah added status/2-code-review and removed status/0-triage labels Oct 4, 2019

thaJeztah requested changes Oct 4, 2019

View reviewed changes

aiordache force-pushed the app-214_client_info_load_func branch 2 times, most recently from ac3f562 to 843aa5c Compare November 7, 2019 09:01

GordonTheTurtle added the dco/no label Nov 7, 2019

aiordache force-pushed the app-214_client_info_load_func branch from 843aa5c to 12d90c6 Compare November 7, 2019 09:04

GordonTheTurtle removed the dco/no label Nov 7, 2019

aiordache requested review from silvin-lubecki and thaJeztah November 7, 2019 09:11

aiordache force-pushed the app-214_client_info_load_func branch from 3aa1869 to 429144d Compare November 7, 2019 10:58

silvin-lubecki reviewed Nov 7, 2019

View reviewed changes

cli/command/cli.go Outdated Show resolved Hide resolved

silvin-lubecki approved these changes Nov 8, 2019

View reviewed changes

aiordache force-pushed the app-214_client_info_load_func branch from 41d2a2b to f98de1c Compare November 8, 2019 09:50

aiordache force-pushed the app-214_client_info_load_func branch from f98de1c to 22a5dad Compare November 8, 2019 10:39

silvin-lubecki merged commit 37f9a88 into docker:master Nov 8, 2019

rumpl mentioned this pull request Nov 8, 2019

Make flag cnab-bunle-json experimental docker-archive-public/docker.app#731

Merged

thaJeztah added this to the 20.03.0 milestone Feb 19, 2020

thaJeztah mentioned this pull request Apr 20, 2020

[19.03 backport] cli: perform feature detection lazily #2457

Merged

thaJeztah added the process/cherry-picked label Apr 20, 2020

thaJeztah mentioned this pull request Jun 15, 2021

docker version get different value docker/for-linux#1251

Closed

3 tasks

	if opts.ConfigDir != "" {
	cliconfig.SetDir(opts.ConfigDir)
	}

	if opts.Common.Debug {
	debug.Enable()
	}

	cli.configFile = cliconfig.LoadDefaultConfigFile(cli.err)

APP-214 Load Client info in getter function #2095

APP-214 Load Client info in getter function #2095

Uh oh!

Conversation

aiordache commented Sep 20, 2019

Uh oh!

silvin-lubecki left a comment

Choose a reason for hiding this comment

Uh oh!

silvin-lubecki Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GordonTheTurtle commented Nov 7, 2019

Uh oh!

Uh oh!

silvin-lubecki left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thaJeztah commented Oct 4, 2019 •

edited

Loading