feat: remove dependency of `docker-credentials-helper` and support context #68

wangxiaoxuan273 · 2023-05-24T09:54:22Z

Resolves #67

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

codecov-commenter · 2023-05-31T02:39:18Z

Codecov Report

Merging #68 (ecb34fe) into main (97227b1) will decrease coverage by 0.45%.
The diff coverage is 66.66%.

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

❗ 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      #68      +/-   ##
==========================================
- Coverage   81.13%   80.68%   -0.45%     
==========================================
  Files           6        6              
  Lines         371      378       +7     
==========================================
+ Hits          301      305       +4     
- Misses         48       49       +1     
- Partials       22       24       +2

Impacted Files	Coverage Δ
native_store.go	`73.91% <66.66%> (-3.02%)`	⬇️

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

main/main.go

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

native_store.go

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

native_store.go

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

internal/executer/executer.go

shizhMSFT · 2023-06-06T08:32:24Z

internal/executer/executer.go

+}
+
+// NewExecuter returns a new Executer instance.
+func NewExecuter(name string) Executer {


It would be better to be used as executer.New().

Suggested change

func NewExecuter(name string) Executer {

func New(name string) Executer {

Renamed the function

shizhMSFT · 2023-06-06T15:22:41Z

native_store.go

+	ServerURL string
+	Username  string
+	Secret    string


It is better to explicitly declare the json field name. For example:

ServerURL string `json:"ServerURL"`

Added the json field name.

shizhMSFT · 2023-06-06T15:23:11Z

native_store.go

+// dockerCredentials mimics how docker credential helper binaries store
+// credential information.


It would be better to have a reference link like https://docs.docker.com/engine/reference/commandline/login/#credential-helper-protocol

Added the reference link.

shizhMSFT · 2023-06-06T15:25:53Z

native_store.go

 // nativeStore implements a credentials store using native keychain to keep
 // credentials secure.
 type nativeStore struct {
-	programFunc client.ProgramFunc
+	executer.Executer


You need to give this component name like

Suggested change

executer.Executer

exec executer.Executer

Otherwise, all public method of executer.Executer will be exposed to the nativeStore. In other words, some one can do

store := NewNativeStore("foobar") exec := store.(executer.Executer) exec.Execute(ctx, "hello", "get")

which should not be allowed.

Added back the variable name.

shizhMSFT · 2023-06-06T15:28:39Z

native_store.go

+	dockerCred := &dockerCredentials{
+		ServerURL: serverAddress,
+	}


What's the meaning to pre-fill the content as Decode will clean it anyway.

Consider the following code

var dockerCred dockerCredentials if err := json.Unmarshal(out, &dockerCred); err != nil {

Replaced with the code above.

shizhMSFT · 2023-06-06T15:29:45Z

native_store.go

+	buffer := new(bytes.Buffer)
+	if err := json.NewEncoder(buffer).Encode(dockerCred); err != nil {
+		return err
+	}
+	_, err := ns.Execute(ctx, buffer, "store")


Consider using json.Marshal() and bytes.NewReader().

Updated accordingly.

shizhMSFT · 2023-06-06T15:38:11Z

@wangxiaoxuan273 The code coverage drops. You may want to have a net gain on it.

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

Wwwsylvia · 2023-06-07T06:32:02Z

internal/executer/executer.go

+
+// NewExecuter returns a new Executer instance.
+func NewExecuter(name string) Executer {
+	return &executable{


A question: Do weed need to check if the executable can be found? Like using exec.LookPath?

I think we need more discussion on that? This is worth a future "feat" PR. I'll leave it as it is for this PR.

It is a good idea and is necessary for detecting desktop.exe. However, we can leave it in the next PR.

wangxiaoxuan273 · 2023-06-08T02:53:53Z

@wangxiaoxuan273 The code coverage drops. You may want to have a net gain on it.

I looked at the CodeDev report, currently all parts of native_store.go are covered, except the error handling parts of json.Marshall/Unmarshall and Execute. I don't think unit tests for these functions are necessary, just to increase the code coverage.

internal/executer/executer.go

shizhMSFT · 2023-06-08T05:30:54Z

internal/executer/executer.go

+
+// NewExecuter returns a new Executer instance.
+func NewExecuter(name string) Executer {
+	return &executable{


It is a good idea and is necessary for detecting desktop.exe. However, we can leave it in the next PR.

native_store.go

shizhMSFT · 2023-06-08T05:35:22Z

I don't think unit tests for these functions are necessary, just to increase the code coverage.

@wangxiaoxuan273 It is necessary. Unit tests are not just for positive paths. Negative paths are equivalently important.

Signed-off-by: wangxiaoxuan273 <[email protected]>

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

shizhMSFT

LGTM

Wwwsylvia

LGTM

Wwwsylvia

LGTM

Fixing the error of PR #68 --------- Signed-off-by: wangxiaoxuan273 <[email protected]>

wangxiaoxuan273 requested review from shizhMSFT, Wwwsylvia and TerryHowe as code owners May 24, 2023 09:54

wangxiaoxuan273 marked this pull request as draft May 29, 2023 05:56

wangxiaoxuan273 added 4 commits May 31, 2023 02:36

added package client, use code from docker-cred

60a145c

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

done unit test

0c407a7

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

added license

fe49d73

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

reset my branch

eea06fa

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

wangxiaoxuan273 added 2 commits June 1, 2023 15:40

temp admit to test locally

46b532c

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

temp commit to local test

0078759

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

github-advanced-security bot found potential problems Jun 1, 2023

View reviewed changes

main/main.go Fixed Show fixed Hide fixed

should not stage main

d0639f3

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

qweeah mentioned this pull request Jun 5, 2023

Remove dependency on docker CLI oras-project/oras#414

Closed

wangxiaoxuan273 added 5 commits June 5, 2023 05:59

added interface code

8c2d0b8

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

local test

56af570

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

locally tested

93bc037

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

for local test

91e9ec3

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

done original unit tests

f0af3dd

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

wangxiaoxuan273 marked this pull request as ready for review June 5, 2023 13:00

clean up doc

6cea0ef

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

wangxiaoxuan273 commented Jun 6, 2023

View reviewed changes

native_store.go Outdated Show resolved Hide resolved

removed an extra comment

147aedf

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

qweeah reviewed Jun 6, 2023

View reviewed changes

native_store.go Outdated Show resolved Hide resolved

qweeah reviewed Jun 6, 2023

View reviewed changes

native_store.go Show resolved Hide resolved

wangxiaoxuan273 added 4 commits June 6, 2023 08:45

implemented docker credential struct

5f48d1c

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

added back the const

6592b51

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

removed exe variable name

94da9e1

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

fixed failed unit tests

714e967

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

shizhMSFT reviewed Jun 6, 2023

View reviewed changes

resolved all comments but the last one

da11fc1

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

Wwwsylvia reviewed Jun 7, 2023

View reviewed changes

shizhMSFT reviewed Jun 8, 2023

View reviewed changes

wangxiaoxuan273 mentioned this pull request Jun 8, 2023

Better error prompt when Docker Desktop is installed #26

Closed

1 task

wangxiaoxuan273 added 3 commits June 8, 2023 16:54

resolved comments

4756dcc

Signed-off-by: wangxiaoxuan273 <[email protected]>

added back variable name

e55f920

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

added unit tests

691b18c

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

shizhMSFT approved these changes Jun 8, 2023

View reviewed changes

Wwwsylvia approved these changes Jun 8, 2023

View reviewed changes

Wwwsylvia merged commit ab5d0bd into oras-project:main Jun 8, 2023

wangxiaoxuan273 mentioned this pull request Jun 8, 2023

fix: added back handling no credentials error #74

Merged

shizhMSFT pushed a commit that referenced this pull request Jun 8, 2023

fix: added back handling no credentials error (#74)

323e44c

Fixing the error of PR #68 --------- Signed-off-by: wangxiaoxuan273 <[email protected]>

This was referenced Jun 15, 2023

Need to check the existence of binaries before creating native store. #78

Closed

Vote for release of v0.2.0 #79

Closed

wangxiaoxuan273 changed the title ~~feat: add package client, the Program type and its related operations~~ feat: remove dependency of docker-credentials-helper and support context Jun 20, 2023

wangxiaoxuan273 deleted the infra branch June 20, 2023 06:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remove dependency of `docker-credentials-helper` and support context #68

feat: remove dependency of `docker-credentials-helper` and support context #68

wangxiaoxuan273 commented May 24, 2023 •

edited

Loading

codecov-commenter commented May 31, 2023 •

edited

Loading

shizhMSFT Jun 6, 2023

wangxiaoxuan273 Jun 7, 2023

shizhMSFT Jun 6, 2023

wangxiaoxuan273 Jun 7, 2023

shizhMSFT Jun 6, 2023

wangxiaoxuan273 Jun 7, 2023

shizhMSFT Jun 6, 2023

wangxiaoxuan273 Jun 7, 2023

shizhMSFT Jun 6, 2023

wangxiaoxuan273 Jun 7, 2023

shizhMSFT Jun 6, 2023

wangxiaoxuan273 Jun 7, 2023

shizhMSFT commented Jun 6, 2023

Wwwsylvia Jun 7, 2023 •

edited

Loading

wangxiaoxuan273 Jun 8, 2023

shizhMSFT Jun 8, 2023

wangxiaoxuan273 commented Jun 8, 2023 •

edited

Loading

shizhMSFT Jun 8, 2023

shizhMSFT commented Jun 8, 2023 •

edited

Loading

shizhMSFT left a comment

Wwwsylvia left a comment

Wwwsylvia left a comment

	func NewExecuter(name string) Executer {
	func New(name string) Executer {

		// dockerCredentials mimics how docker credential helper binaries store
		// credential information.

feat: remove dependency of docker-credentials-helper and support context #68

feat: remove dependency of docker-credentials-helper and support context #68

Conversation

wangxiaoxuan273 commented May 24, 2023 • edited Loading

codecov-commenter commented May 31, 2023 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shizhMSFT commented Jun 6, 2023

Wwwsylvia Jun 7, 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 8, 2023 • edited Loading

Choose a reason for hiding this comment

shizhMSFT commented Jun 8, 2023 • edited Loading

shizhMSFT left a comment

Choose a reason for hiding this comment

Wwwsylvia left a comment

Choose a reason for hiding this comment

Wwwsylvia left a comment

Choose a reason for hiding this comment

feat: remove dependency of `docker-credentials-helper` and support context #68

feat: remove dependency of `docker-credentials-helper` and support context #68

wangxiaoxuan273 commented May 24, 2023 •

edited

Loading

codecov-commenter commented May 31, 2023 •

edited

Loading

Wwwsylvia Jun 7, 2023 •

edited

Loading

wangxiaoxuan273 commented Jun 8, 2023 •

edited

Loading

shizhMSFT commented Jun 8, 2023 •

edited

Loading