Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: make credentials.NewMemoryStore return an interface #605

Merged
merged 1 commit into from
Sep 25, 2023

Conversation

Wwwsylvia
Copy link
Member

@Wwwsylvia Wwwsylvia commented Sep 22, 2023

  1. Un-expose credentials.MemoryStore as it is unecessary to be public
  2. Make credentials.NewMemoryStore return an interface instead of a struct

Signed-off-by: Lixia (Sylvia) Lei <lixlei@microsoft.com>
@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2023

Codecov Report

Merging #605 (0a6db83) into main (9f83e67) will decrease coverage by 0.19%.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main     #605      +/-   ##
==========================================
- Coverage   74.64%   74.45%   -0.19%     
==========================================
  Files          59       59              
  Lines        5266     5266              
==========================================
- Hits         3931     3921      -10     
- Misses        983      991       +8     
- Partials      352      354       +2     
Files Changed Coverage Δ
registry/remote/credentials/memory_store.go 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@Wwwsylvia
Copy link
Member Author

Wwwsylvia commented Sep 22, 2023

@uanid Please take a look at this change

Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@uanid
Copy link
Contributor

uanid commented Sep 22, 2023

@uanid Please take a look at this change

How about changing FileStore to a private too?
If we add the DisablePut property to NewFileStore? Then, we could make it private.
Changing the public API could be an issue, but since you moved the code to oras-go and package path is changed, some changes now seem okay.

@TerryHowe
Copy link
Member

TerryHowe commented Sep 22, 2023

@uanid Please take a look at this change

How about changing FileStore to a private too? If we add the DisablePut property to NewFileStore? Then, we could make it private. Changing the public API could be an issue, but since you moved the code to oras-go and package path is changed, some changes now seem okay.

Sounds like a good idea, but subject of another PR, although I don't know if it is practical.

Copy link
Member

@TerryHowe TerryHowe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@uanid
Copy link
Contributor

uanid commented Sep 22, 2023

@uanid Please take a look at this change

LGTM too

@Wwwsylvia
Copy link
Member Author

How about changing FileStore to a private too?
If we add the DisablePut property to NewFileStore? Then, we could make it private.
Changing the public API could be an issue, but since you moved the code to oras-go and package path is changed, some changes now seem okay.

But that would require users to specify an extra parameter on initialization. I think it's ok to keep FileStore exposed for convenience. @shizhMSFT What do you think?

@Wwwsylvia Wwwsylvia merged commit cb8c8bc into oras-project:main Sep 25, 2023
7 checks passed
@Wwwsylvia Wwwsylvia deleted the creds_memory branch September 25, 2023 03:08
@shizhMSFT
Copy link
Contributor

But that would require users to specify an extra parameter on initialization. I think it's ok to keep FileStore exposed for convenience.

The point of having FileStore exposed is to make the property changeable after initialization.

@Wwwsylvia Wwwsylvia mentioned this pull request Jan 26, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants