fix: use a better error message on WSL #77

wangxiaoxuan273 · 2023-06-15T02:55:04Z

Resolves #26

codecov-commenter · 2023-06-15T02:57:01Z

Codecov Report

Merging #77 (a8175a5) into main (bf5244c) will not change coverage.
The diff coverage is 50.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##             main      #77   +/-   ##
=======================================
  Coverage   82.73%   82.73%           
=======================================
  Files           6        6           
  Lines         388      388           
=======================================
  Hits          321      321           
  Misses         45       45           
  Partials       22       22

Impacted Files	Coverage Δ
registry.go	`83.33% <50.00%> (ø)`

wangxiaoxuan273 · 2023-06-15T03:42:35Z

Changed the displayed error message of the current scenario.

FeynmanZhou · 2023-06-15T03:46:59Z

native_store.go

@@ -120,6 +122,9 @@ func (ns *nativeStore) Put(ctx context.Context, serverAddress string, cred auth.
 		return err
 	}
 	_, err = ns.exec.Execute(ctx, bytes.NewReader(credJSON), "store")
+	if err != nil && err.Error() == errExeNotInPathMessage {
+		return fmt.Errorf("credential store is configured to `desktop` but docker desktop seems not running")


Should we call it credentials store by following the consistent naming with Docker? See https://docs.docker.com/engine/reference/commandline/login/#credentials-store

Will this error message Error: error storing credentials: be added by ORAS to the beginning in this output?

Will this error message Error: error storing credentials: be added by ORAS to the beginning in this output?

You can take a look at the screen shot above. It looks like the answer is no. We can change it if we need. @FeynmanZhou

Should we call it credentials store by following the consistent naming with Docker? See https://docs.docker.com/engine/reference/commandline/login/#credentials-store

Nice catch. Changed the name.

@wangxiaoxuan273 You can check the log style in other ORAS error logs. If there is a consistent log style in ORAS, we may need to follow it and add it accordingly. If not, we can skip it.

native_store.go

TerryHowe · 2023-06-15T10:30:00Z

native_store.go

@@ -30,6 +31,7 @@ const (
 	remoteCredentialsPrefix       = "docker-credential-"
 	emptyUsername                 = "<token>"
 	errCredentialsNotFoundMessage = "credentials not found in native keychain"
+	errExeNotInPathMessage        = "exec: \"docker-credential-desktop.exe\": executable file not found in $PATH"


Should there be a native_store_wsl.go to handle this that or can we count on a generic match?

We don't think a separate wsl.go file is necessary. We have changed exact string matching to error matching to improve the code.

The code was improved by the time this was merged, but this is exactly the kind of thing you'd want to put in a platform specific module.

shizhMSFT · 2023-06-16T08:07:05Z

native_store.go

@@ -30,6 +31,7 @@ const (
 	remoteCredentialsPrefix       = "docker-credential-"
 	emptyUsername                 = "<token>"
 	errCredentialsNotFoundMessage = "credentials not found in native keychain"
+	errExeNotInPathMessage        = "exec: \"docker-credential-desktop.exe\": executable file not found in $PATH"


Consider adding the following code to executer.go.

if execErr, ok := err.(*exec.Error); ok && execErr.Err == exec.ErrNotFound { return nil, errors.New("credentials store is configured to `desktop` but Docker Desktop seems not running") }

Added this part. It's a very good suggestion.

shizhMSFT · 2023-06-16T08:32:13Z

native_store.go

@@ -120,6 +122,9 @@ func (ns *nativeStore) Put(ctx context.Context, serverAddress string, cred auth.
 		return err
 	}
 	_, err = ns.exec.Execute(ctx, bytes.NewReader(credJSON), "store")
+	if err != nil && err.Error() == errExeNotInPathMessage {
+		return fmt.Errorf("credentials store is configured to `desktop` but Docker Desktop seems not running")


nit:

Suggested change

return fmt.Errorf("credentials store is configured to `desktop` but Docker Desktop seems not running")

return fmt.Errorf("credentials store is configured to `desktop.exe` but Docker Desktop seems not running")

/cc @FeynmanZhou

@shizhMSFT Good catch.

shizhMSFT · 2023-06-19T07:58:09Z

internal/executer/executer.go

 				err = errors.New(errMessage)
+				return nil, err


nit: clean code

Suggested change

err = errors.New(errMessage)

return nil, err

return nil, errors.New(errMessage)

changed this part and used a switch statement.

shizhMSFT · 2023-06-19T07:59:05Z

internal/executer/executer.go

+		// check if the error is caused by Docker Desktop not running
+		if execErr, ok := err.(*exec.Error); ok && execErr.Err == exec.ErrNotFound {
+			return nil, errors.New("credentials store is configured to `desktop` but Docker Desktop seems not running")
+		}


What if the binary is not docker-credential-desktop.exe?

added a third condition.

wangxiaoxuan273 · 2023-06-19T08:36:28Z

Current output:

FeynmanZhou · 2023-06-19T08:37:56Z

Current output:

This version looks good to me. Thanks @wangxiaoxuan273

Signed-off-by: Xiaoxuan Wang <[email protected]>

shizhMSFT

LGTM

wangxiaoxuan273 requested review from shizhMSFT, Wwwsylvia and TerryHowe as code owners June 15, 2023 02:55

wangxiaoxuan273 changed the title ~~fix: use a error message on WSL~~ fix: use a better error message on WSL Jun 15, 2023

FeynmanZhou reviewed Jun 15, 2023

View reviewed changes

native_store.go Outdated Show resolved Hide resolved

TerryHowe reviewed Jun 15, 2023

View reviewed changes

shizhMSFT reviewed Jun 16, 2023

View reviewed changes

wangxiaoxuan273 requested a review from shizhMSFT June 19, 2023 07:30

shizhMSFT reviewed Jun 19, 2023

View reviewed changes

use better error message

a8175a5

Signed-off-by: Xiaoxuan Wang <[email protected]>

shizhMSFT approved these changes Jun 19, 2023

View reviewed changes

shizhMSFT merged commit 26b25ce into oras-project:main Jun 19, 2023

wangxiaoxuan273 deleted the err-msg branch June 19, 2023 09:14

wangxiaoxuan273 mentioned this pull request Jun 19, 2023

Vote for release of v0.2.0 #79

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use a better error message on WSL #77

fix: use a better error message on WSL #77

wangxiaoxuan273 commented Jun 15, 2023

codecov-commenter commented Jun 15, 2023 •

edited

Loading

wangxiaoxuan273 commented Jun 15, 2023

FeynmanZhou Jun 15, 2023

FeynmanZhou Jun 15, 2023

wangxiaoxuan273 Jun 15, 2023

wangxiaoxuan273 Jun 15, 2023

FeynmanZhou Jun 19, 2023

TerryHowe Jun 15, 2023

wangxiaoxuan273 Jun 19, 2023

TerryHowe Jun 19, 2023

shizhMSFT Jun 16, 2023

wangxiaoxuan273 Jun 19, 2023 •

edited

Loading

shizhMSFT Jun 16, 2023

FeynmanZhou Jun 17, 2023

shizhMSFT Jun 19, 2023

wangxiaoxuan273 Jun 19, 2023 •

edited

Loading

shizhMSFT Jun 19, 2023

wangxiaoxuan273 Jun 19, 2023

wangxiaoxuan273 commented Jun 19, 2023

FeynmanZhou commented Jun 19, 2023

shizhMSFT left a comment

	return fmt.Errorf("credentials store is configured to `desktop` but Docker Desktop seems not running")
	return fmt.Errorf("credentials store is configured to `desktop.exe` but Docker Desktop seems not running")

	err = errors.New(errMessage)
	return nil, err
	return nil, errors.New(errMessage)

fix: use a better error message on WSL #77

fix: use a better error message on WSL #77

Conversation

wangxiaoxuan273 commented Jun 15, 2023

codecov-commenter commented Jun 15, 2023 • edited Loading

Codecov Report

wangxiaoxuan273 commented Jun 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangxiaoxuan273 Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangxiaoxuan273 Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangxiaoxuan273 commented Jun 19, 2023

FeynmanZhou commented Jun 19, 2023

shizhMSFT left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 15, 2023 •

edited

Loading

wangxiaoxuan273 Jun 19, 2023 •

edited

Loading

wangxiaoxuan273 Jun 19, 2023 •

edited

Loading