fix: use a better error message on WSL by wangxiaoxuan273 · Pull Request #77 · oras-project/oras-credentials-go

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

 	}
 	_, 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.

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

 	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

 	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

 	}
 	_, 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

 				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

+		// 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 <wangxiaoxuan119@gmail.com>

shizhMSFT

LGTM

wangxiaoxuan273 requested review from TerryHowe, Wwwsylvia and shizhMSFT 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

Comment thread native_store.go Outdated

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 <wangxiaoxuan119@gmail.com>

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

	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)

Conversation

wangxiaoxuan273 commented Jun 15, 2023

Uh oh!

codecov-commenter commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wangxiaoxuan273 commented Jun 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangxiaoxuan273 Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangxiaoxuan273 Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangxiaoxuan273 commented Jun 19, 2023

Uh oh!

FeynmanZhou commented Jun 19, 2023

Uh oh!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Jun 15, 2023 •

edited

Loading

wangxiaoxuan273 Jun 19, 2023 •

edited

Loading

wangxiaoxuan273 Jun 19, 2023 •

edited

Loading