feat: implement StoreWithFallbacks #46

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

Resolves #35

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

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 <[email protected]>

store_with_fallbacks.go

store_with_fallbacks_test.go

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

Wwwsylvia

We may need better documentations for the behaviors.

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

store_with_fallbacks.go

+
+// 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 <[email protected]>

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

store_with_fallbacks.go

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

store_with_fallbacks.go

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

store_with_fallbacks.go

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

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

store_with_fallbacks.go

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

store_with_fallbacks.go

+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 <[email protected]>

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

store.go

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


This looks unnecessary.

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

store.go

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

store.go

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

store.go

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


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

Wwwsylvia

LGTM

shizhMSFT

LGTM

implemented fallback store

7dc6c2f

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

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

added tests

b35654e

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

Wwwsylvia reviewed Apr 17, 2023

View reviewed changes

store_with_fallbacks.go Outdated Show resolved Hide resolved

store_with_fallbacks_test.go Outdated Show resolved Hide resolved

store_with_fallbacks_test.go Outdated Show resolved Hide resolved

resolved comments

d4bad0a

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

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 <[email protected]>

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 <[email protected]>

resolved comments

3678f50

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

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 <[email protected]>

refined comments

e2e7826

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement StoreWithFallbacks #46

feat: implement StoreWithFallbacks #46

wangxiaoxuan273 commented Apr 17, 2023

codecov-commenter commented Apr 17, 2023 •

edited

Loading

Wwwsylvia left a comment •

edited

Loading

Wwwsylvia Apr 19, 2023 •

edited

Loading

Wwwsylvia Apr 19, 2023

wangxiaoxuan273 commented Apr 19, 2023

Wwwsylvia Apr 19, 2023

wangxiaoxuan273 Apr 20, 2023

Wwwsylvia Apr 19, 2023

wangxiaoxuan273 Apr 20, 2023

shizhMSFT Apr 19, 2023

wangxiaoxuan273 Apr 20, 2023

shizhMSFT Apr 19, 2023

wangxiaoxuan273 Apr 20, 2023

shizhMSFT Apr 19, 2023

wangxiaoxuan273 Apr 20, 2023

Wwwsylvia Apr 20, 2023

wangxiaoxuan273 Apr 20, 2023

Wwwsylvia Apr 20, 2023

wangxiaoxuan273 Apr 20, 2023

Wwwsylvia Apr 20, 2023

wangxiaoxuan273 Apr 20, 2023

Wwwsylvia Apr 20, 2023

wangxiaoxuan273 Apr 20, 2023

Wwwsylvia left a comment

shizhMSFT left a comment

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

feat: implement StoreWithFallbacks #46

feat: implement StoreWithFallbacks #46

Conversation

wangxiaoxuan273 commented Apr 17, 2023

codecov-commenter commented Apr 17, 2023 • edited Loading

Codecov Report

Wwwsylvia left a comment • edited Loading

Choose a reason for hiding this comment

Wwwsylvia Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

Wwwsylvia left a comment

Choose a reason for hiding this comment

shizhMSFT left a comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 17, 2023 •

edited

Loading

Wwwsylvia left a comment •

edited

Loading

Wwwsylvia Apr 19, 2023 •

edited

Loading