feat: remove dependency of `docker-credentials-helper` and support context by wangxiaoxuan273 · Pull Request #68 · oras-project/oras-credentials-go

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

Resolves #67

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

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

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

+}
+
+// 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

+	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

+// 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

 // 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

+	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

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

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

+
+// 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.

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

+
+// 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.

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

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

shizhMSFT

LGTM

Wwwsylvia

LGTM

Wwwsylvia

LGTM

Fixing the error of PR #68 --------- Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

wangxiaoxuan273 requested review from TerryHowe, Wwwsylvia and shizhMSFT 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 <wangxiaoxuan119@gmail.com>

done unit test

0c407a7

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

added license

fe49d73

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

reset my branch

eea06fa

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

wangxiaoxuan273 added 2 commits June 1, 2023 15:40

temp admit to test locally

46b532c

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

temp commit to local test

0078759

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

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

View reviewed changes

Comment thread main/main.go Fixed

should not stage main

d0639f3

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

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

local test

56af570

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

locally tested

93bc037

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

for local test

91e9ec3

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

done original unit tests

f0af3dd

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

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

clean up doc

6cea0ef

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

wangxiaoxuan273 commented Jun 6, 2023

View reviewed changes

Comment thread native_store.go Outdated

removed an extra comment

147aedf

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

qweeah reviewed Jun 6, 2023

View reviewed changes

Comment thread native_store.go Outdated

qweeah reviewed Jun 6, 2023

View reviewed changes

Comment thread native_store.go

wangxiaoxuan273 added 4 commits June 6, 2023 08:45

implemented docker credential struct

5f48d1c

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

added back the const

6592b51

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

removed exe variable name

94da9e1

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

fixed failed unit tests

714e967

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

shizhMSFT reviewed Jun 6, 2023

View reviewed changes

resolved all comments but the last one

da11fc1

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

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

added back variable name

e55f920

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

added unit tests

691b18c

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

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

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

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

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

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

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!

shizhMSFT commented Jun 6, 2023

Uh oh!

Wwwsylvia Jun 7, 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 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Wwwsylvia left a comment

Choose a reason for hiding this comment

Uh oh!

Wwwsylvia left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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