feat: implement StoreWithFallbacks by wangxiaoxuan273 · Pull Request #46 · oras-project/oras-credentials-go

wangxiaoxuan273 · 2023-04-17T06:56:30Z

Resolves #35

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

codecov-commenter · 2023-04-17T07:01:59Z

Codecov Report

Merging #46 (3678f50) into main (87a3ee4) will decrease coverage by 0.65%.
The diff coverage is 71.42%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
- Coverage   78.32%   77.67%   -0.65%     
==========================================
  Files           3        4       +1     
  Lines         203      224      +21     
==========================================
+ Hits          159      174      +15     
- Misses         33       37       +4     
- Partials       11       13       +2

Impacted Files	Coverage Δ
store.go	`71.42% <71.42%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

Wwwsylvia

We may need better documentations for the behaviors.

Wwwsylvia · 2023-04-19T06:54:12Z

+
+// NewStoreWithFallbacks returns a new store based on the given stores.
+// The second and the subsequent stores will be used as fallbacks for the first store.
+func NewStoreWithFallbacks(store Store, fallbacks ...Store) Store {


I'm still struggling between

NewStoreWithFallbacks(store Store, fallbacks ...Store) Store

and

NewStoreWithFallbacks(stores ...Store) Store

Later we will have a docker store which is based on the docker configuration file at the default location. I'm wondering how we should use it together with StoreWithFallbacks.

If we go for option 1, the experience would be:

ns := NewNativeStore() store := NewStoreWithFallbacks(ns, NewStoreFromDocker())

or

dockerStore := NewStoreFromDocker() store := NewStoreWithFallbacks(dockerStore)

If we go for option 2, we should have the docker store to be the default store when no store is passed.
So

dockerStore := NewStoreFromDocker() store := NewStoreWithFallbacks(dockerStore)

is equivalent to

store := NewStoreWithFallbacks()

I think I still prefer option 1.

wangxiaoxuan273 · 2023-04-19T08:15:03Z

We may need better documentations for the behaviors.

Good advice. Added more detailed documentations for Get, Store and Delete.

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

Wwwsylvia · 2023-04-19T09:30:38Z

+}
+
+// NewStoreWithFallbacks returns a new store based on the given stores.
+// The first store in the array is used as the primary store. The second


The first store in the array is used as the primary store

This looks a bit strange as this function does not accept an array of stores for the primary store.

removed the description about the array.

Wwwsylvia · 2023-04-19T09:31:25Z

+}
+
+// Get retrieves credentials from the StoreWithFallbacks for the given server.
+// It searches the array of stores for the credentials of serverAddress


The array of stores is invisible to users.

removed the description about the array.

shizhMSFT · 2023-04-19T16:50:23Z

Can we put everything in store.go instead of having a new file?

shizhMSFT · 2023-04-19T16:51:07Z

+// StoreWithFallbacks is a store that has multiple fallback stores.
+// Please use the NewStoreWithFallbacks to create new instances of
+// StoreWithFallbacks.
+type StoreWithFallbacks struct {


Do we need to export this struct? Can we make it private?

Made it private.

shizhMSFT · 2023-04-19T16:52:46Z

+func NewStoreWithFallbacks(store Store, fallbacks ...Store) Store {
+	return &StoreWithFallbacks{
+		stores: append([]Store{store}, fallbacks...),
+	}
+}


If len(fallbacks) == 0, should we just return store?

changed it.

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

Wwwsylvia · 2023-04-20T08:14:38Z

+// Please use the NewStoreWithFallbacks to create new instances of
+// storeWithFallbacks.


This looks unnecessary.

Wwwsylvia · 2023-04-20T08:15:34Z

+// The first store is used as the primary store. The second and the
+// subsequent stores will be used as fallbacks for the first store.
+func NewStoreWithFallbacks(store Store, fallbacks ...Store) Store {
+	if fallbacks == nil {


Suggested change

if fallbacks == nil {

if len(fallbacks) == 0 {

fallbacks can be not-nil but empty.

Wwwsylvia · 2023-04-20T08:17:00Z

+// It searches the primary and the fallback stores for the credentials of serverAddress
+// and returns when it finds the credentials in any of the stores.


I'm wondering if users can see these comments. We may need to move them to NewStoreWithFallbacks.

Wwwsylvia · 2023-04-20T08:17:12Z

+}
+
+// Put saves credentials into the StoreWithFallbacks. It puts
+// the credentials into the primary store.


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

Wwwsylvia

LGTM

shizhMSFT

LGTM

implemented fallback store

7dc6c2f

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

wangxiaoxuan273 requested review from TerryHowe, Wwwsylvia and shizhMSFT as code owners April 17, 2023 06:56

added tests

b35654e

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

Wwwsylvia reviewed Apr 17, 2023

View reviewed changes

Comment thread store_with_fallbacks.go Outdated

Comment thread store_with_fallbacks_test.go Outdated

Comment thread store_with_fallbacks_test.go Outdated

resolved comments

d4bad0a

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

wangxiaoxuan273 requested a review from Wwwsylvia April 18, 2023 08:02

Wwwsylvia reviewed Apr 19, 2023

View reviewed changes

refined doc comments

e42f60c

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

Wwwsylvia reviewed Apr 19, 2023

View reviewed changes

shizhMSFT reviewed Apr 19, 2023

View reviewed changes

wangxiaoxuan273 added 2 commits April 20, 2023 02:30

removed array descriptions

00eba81

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

resolved comments

3678f50

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

Wwwsylvia suggested changes Apr 20, 2023

View reviewed changes

Wwwsylvia mentioned this pull request Apr 20, 2023

feat: Support creating store from a config file #47

Merged

wangxiaoxuan273 added 2 commits April 20, 2023 08:32

resolved comments

69df6cd

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

refined comments

e2e7826

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

Wwwsylvia approved these changes Apr 20, 2023

View reviewed changes

shizhMSFT approved these changes Apr 20, 2023

View reviewed changes

shizhMSFT merged commit e6ef977 into oras-project:main Apr 20, 2023

wangxiaoxuan273 deleted the fallback branch April 20, 2023 09:10

		// Please use the NewStoreWithFallbacks to create new instances of
		// storeWithFallbacks.

		// It searches the primary and the fallback stores for the credentials of serverAddress
		// and returns when it finds the credentials in any of the stores.

Conversation

wangxiaoxuan273 commented Apr 17, 2023

Uh oh!

codecov-commenter commented Apr 17, 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!

Wwwsylvia left a comment • 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!

Wwwsylvia Apr 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!

wangxiaoxuan273 commented Apr 19, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Wwwsylvia left a comment

Choose a reason for hiding this comment

Uh oh!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Apr 17, 2023 •

edited

Loading

Wwwsylvia left a comment •

edited

Loading

Wwwsylvia Apr 19, 2023 •

edited

Loading