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

context use: don't create/update config file and directories if not needed #3721

Merged
merged 2 commits into from
Jul 29, 2022

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jul 29, 2022

context use: skip validation for "default" context

This code was handling validation and parsing, only to discard the results if it was the default context.

context use: don't create/update config file and directories if not needed

Avoid updating the config-file if nothing changed. This also prevents creating
the file and config-directory if the default is used and no config-file existed
yet.

config.Save() performs various steps (creating the directory, updating
or copying permissions, etc etc), which are not needed if the defaults are
used;

// Save encodes and writes out all the authorization information
func (configFile *ConfigFile) Save() (retErr error) {
if configFile.Filename == "" {
return errors.Errorf("Can't save config with empty filename")
}
dir := filepath.Dir(configFile.Filename)
if err := os.MkdirAll(dir, 0700); err != nil {
return err
}
temp, err := os.CreateTemp(dir, filepath.Base(configFile.Filename))
if err != nil {
return err
}
defer func() {
temp.Close()
if retErr != nil {
if err := os.Remove(temp.Name()); err != nil {
logrus.WithError(err).WithField("file", temp.Name()).Debug("Error cleaning up temp file")
}
}
}()
err = configFile.SaveToWriter(temp)
if err != nil {
return err
}
if err := temp.Close(); err != nil {
return errors.Wrap(err, "error closing temp file")
}
// Handle situation where the configfile is a symlink
cfgFile := configFile.Filename
if f, err := os.Readlink(cfgFile); err == nil {
cfgFile = f
}
// Try copying the current config file (if any) ownership and permissions
copyFilePermissions(cfgFile, temp.Name())
return os.Rename(temp.Name(), cfgFile)
}

- Description for the changelog

The client no longer creates or updates the CLI config file when running `docker context use` and the selected context is the current context.

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

This code was handling validation and parsing, only to discard the
results if it was the default context.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the context_optimisations branch from 6c9c5e8 to fc7daec Compare July 29, 2022 10:25
@thaJeztah thaJeztah marked this pull request as ready for review July 29, 2022 10:25
@thaJeztah thaJeztah requested a review from nicks July 29, 2022 10:25
@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2022

Codecov Report

Merging #3721 (7963778) into master (7963778) will not change coverage.
The diff coverage is n/a.

❗ Current head 7963778 differs from pull request most recent head f87d7ed. Consider uploading reports for the commit f87d7ed to get more accurate results

@@           Coverage Diff           @@
##           master    #3721   +/-   ##
=======================================
  Coverage   59.11%   59.11%           
=======================================
  Files         289      289           
  Lines       24665    24665           
=======================================
  Hits        14581    14581           
  Misses       9212     9212           
  Partials      872      872           

@thaJeztah
Copy link
Member Author

@nicks @rumpl @vvoland PTAL

cli, err := command.NewDockerCli(command.WithCombinedStreams(io.Discard))
assert.NilError(t, err)
assert.NilError(t, newUseCommand(cli).RunE(nil, []string{"default"}))
assert.NilError(t, err)
Copy link
Collaborator

@vvoland vvoland Jul 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant last assert

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch; sloppy copy/paste 😅

fixed!

…eeded

Avoid updating the config-file if nothing changed. This also prevents creating
the file and config-directory if the default is used and no config-file existed
yet.

`config.Save()` performs various steps (creating the directory, updating
or copying permissions, etc etc), which are not needed if the defaults are
used; https://github.com/docker/cli/blob/a445d97c2536f0de37469d0ea9881288d6c49cbf/cli/config/configfile/file.go#L135-L176

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the context_optimisations branch from fc7daec to f87d7ed Compare July 29, 2022 12:20
@thaJeztah
Copy link
Member Author

All green; let's get this one in

@thaJeztah thaJeztah merged commit e198123 into docker:master Jul 29, 2022
@thaJeztah thaJeztah deleted the context_optimisations branch July 29, 2022 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants