Skip to content

Commit

Permalink
fix: error readable by using the right placeholder (runfinch#39)
Browse files Browse the repository at this point in the history
*Description of changes:*

Logs aren't as readable as they could be, I saw this during `finch vm
init`:

```
INFO[0009] sudoers file not found: %!w(*fs.PathError=&{open /etc/sudoers.d/finch-lima 2})
```

From the looks of it, we don't need the `%w` since the current logging
implementation doesn't use `fmt.Errorf` under the hood anyway, and thus
makes the output hard to read without the only potential benefit being
utilised (unwrapping). And since the logging interface's methods
themselves don't return anything, it's fairly likely unwrapping won't
happen in any other implementation (there isn't even a reason for it
anyway - the callee will have all the args available to it, so it can
introspect the error passed directly).

(Since this `Infof` call is made right after an `errors.Is`, the whole
`%v`/`%w` bit is redundant as it will just say "sudoers file not found:
no such file or directory", but I didn't want to change error semantics
in this PR.)


*Testing done:*

`make test-unit` and using `logrus` directly to verify none of its `*f`
methods use `fmt.Errorf`.


- [x] I've reviewed the guidance in CONTRIBUTING.md


#### License Acceptance

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.

Signed-off-by: Ondrej Kokes <ondrej.kokes@gmail.com>
Co-authored-by: Weike Qu <47606630+weikequ@users.noreply.github.com>
  • Loading branch information
kokes and weikequ committed Nov 25, 2022
1 parent 4c27cdf commit 8e5f38d
Show file tree
Hide file tree
Showing 6 changed files with 17 additions and 17 deletions.
10 changes: 5 additions & 5 deletions pkg/dependency/vmnet/binaries.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func (bin *binaries) buildArtifactSocketVmnetExe() string {
func (bin *binaries) Installed() bool {
dirExists, err := afero.DirExists(bin.fs, bin.installationPath())
if err != nil {
bin.l.Errorf("failed to get status of binaries directory: %w", err)
bin.l.Errorf("failed to get status of binaries directory: %v", err)
return false
}
if !dirExists {
Expand All @@ -76,18 +76,18 @@ func (bin *binaries) Installed() bool {
buildArtifactFileBytes, err := afero.ReadFile(bin.fs, bin.buildArtifactSocketVmnetExe())
if err != nil {
if errors.Is(err, fs.ErrNotExist) {
bin.l.Infof("dependency socket_vmnet file not found: %w", err)
bin.l.Infof("dependency socket_vmnet file not found: %v", err)
} else {
bin.l.Errorf("failed to read dependency socket_vmnet file: %w", err)
bin.l.Errorf("failed to read dependency socket_vmnet file: %v", err)
}
return false
}
installedFileBytes, err := afero.ReadFile(bin.fs, bin.installationPathSocketVmnetExe())
if err != nil {
if errors.Is(err, fs.ErrNotExist) {
bin.l.Infof("installed socket_vmnet file not found: %w", err)
bin.l.Infof("installed socket_vmnet file not found: %v", err)
} else {
bin.l.Errorf("failed to read installed socket_vmnet file: %w", err)
bin.l.Errorf("failed to read installed socket_vmnet file: %v", err)
}
return false
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/dependency/vmnet/binaries_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func TestBinaries_Installed(t *testing.T) {
pathErr.Path = "mock_prefix/dependencies/lima-socket_vmnet/opt/finch/bin/socket_vmnet"
pathErr.Err = errors.New("file does not exist")

l.EXPECT().Infof("dependency socket_vmnet file not found: %w", &pathErr)
l.EXPECT().Infof("dependency socket_vmnet file not found: %v", &pathErr)
},
want: false,
},
Expand All @@ -125,7 +125,7 @@ func TestBinaries_Installed(t *testing.T) {
pathErr.Path = "/opt/finch/bin/socket_vmnet"
pathErr.Err = errors.New("file does not exist")

l.EXPECT().Infof("installed socket_vmnet file not found: %w", &pathErr)
l.EXPECT().Infof("installed socket_vmnet file not found: %v", &pathErr)
},
want: false,
},
Expand Down
6 changes: 3 additions & 3 deletions pkg/dependency/vmnet/sudoers_file.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,16 @@ func (s *sudoersFile) Installed() bool {
b, err := afero.ReadFile(s.fs, s.path())
if err != nil {
if errors.Is(err, fs.ErrNotExist) {
s.l.Infof("sudoers file not found: %w", err)
s.l.Infof("sudoers file not found: %v", err)
} else {
s.l.Errorf("failed to read sudoers file: %w", err)
s.l.Errorf("failed to read sudoers file: %v", err)
}
return false
}
cmd := s.limaCmdCreator.CreateWithoutStdio("sudoers")
out, err := cmd.Output()
if err != nil {
s.l.Errorf("failed to run lima sudoers command: %w", err)
s.l.Errorf("failed to run lima sudoers command: %v", err)
return false
}
return bytes.Equal(b, out)
Expand Down
4 changes: 2 additions & 2 deletions pkg/dependency/vmnet/sudoers_file_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func TestSudoers_Installed(t *testing.T) {
pathErr.Path = "/etc/sudoers.d/finch-lima"
pathErr.Err = errors.New("file does not exist")

l.EXPECT().Infof("sudoers file not found: %w", &pathErr)
l.EXPECT().Infof("sudoers file not found: %v", &pathErr)
},
want: false,
},
Expand All @@ -73,7 +73,7 @@ func TestSudoers_Installed(t *testing.T) {

lc.EXPECT().CreateWithoutStdio("sudoers").Return(cmd)
cmd.EXPECT().Output().Return([]byte{}, wantErr)
l.EXPECT().Errorf("failed to run lima sudoers command: %w", wantErr)
l.EXPECT().Errorf("failed to run lima sudoers command: %v", wantErr)
},
want: false,
},
Expand Down
6 changes: 3 additions & 3 deletions pkg/dependency/vmnet/update_override_lima_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,16 +62,16 @@ func (overConf *overrideLimaConfig) verifyConfigHasNetworkSection(filePath strin
yamlFile, err := afero.ReadFile(overConf.fs, filePath)
if err != nil {
if errors.Is(err, fs.ErrNotExist) {
overConf.l.Debugf("config file not found: %w", err)
overConf.l.Debugf("config file not found: %v", err)
} else {
overConf.l.Errorf("failed to read config file: %w", err)
overConf.l.Errorf("failed to read config file: %v", err)
}
return false
}
var cfg NetworkConfig
err = yaml.Unmarshal(yamlFile, &cfg)
if err != nil {
overConf.l.Errorf("failed to unmarshal YAML from override config file: %w", err)
overConf.l.Errorf("failed to unmarshal YAML from override config file: %v", err)
return false
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/dependency/vmnet/update_override_lima_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func TestOverrideLimaConfig_verifyConfigHasNetworkSection(t *testing.T) {
pathErr.Path = "mock_config_file"
pathErr.Err = errors.New("file does not exist")

l.EXPECT().Debugf("config file not found: %w", &pathErr)
l.EXPECT().Debugf("config file not found: %v", &pathErr)
},
want: false,
},
Expand All @@ -59,7 +59,7 @@ func TestOverrideLimaConfig_verifyConfigHasNetworkSection(t *testing.T) {
var typeErr yaml.TypeError
typeErr.Errors = []string{"line 1: cannot unmarshal !!str `this is...` into vmnet.NetworkConfig"}

l.EXPECT().Errorf("failed to unmarshal YAML from override config file: %w", &typeErr)
l.EXPECT().Errorf("failed to unmarshal YAML from override config file: %v", &typeErr)
},
want: false,
},
Expand Down

0 comments on commit 8e5f38d

Please sign in to comment.