Notary client refactor - simpler reader interface #1377
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
justincormack
approved these changes
Aug 2, 2018
|
(Test issues are due to GAS being renamed - I’ll fix that in a separate PR) |
client/interface.go
Outdated
| // Repository represents the set of options that must be supported over a TUF repo. | ||
| // Reader represents the set of options that must be supported over a TUF repo for | ||
| // reading | ||
| type Reader interface { |
There was a problem hiding this comment.
Wondering if there is a better name than Reader as that usually means an io.Reader...
There was a problem hiding this comment.
I am happy to name it something else. Alternately we could just make those methods that take a TUF repo instead of having a wrapper
cyli
force-pushed
the
notary-client-refactor
branch
from
3b71249 to
88698de
Compare
endophage
approved these changes
Aug 13, 2018
There was a problem hiding this comment.
LGTM, this is great.
I agree with @justincormack's comment on Reader, maybe call it ReadOnly? Then one would be doing something like
import "github.com/theupdateframework/notary/client"
...
client.NewReadOnly(...)
When combined with the package name I think it's very clear semantics.
cyli
force-pushed
the
notary-client-refactor
branch
from
88698de to
ad5ee33
Compare
|
Thanks for the feedback @justincormack and @endophage! Renamed to |
cyli
force-pushed
the
notary-client-refactor
branch
from
ad5ee33 to
8f41690
Compare
cyli
force-pushed
the
notary-client-refactor
branch
from
8f41690 to
4096cf4
Compare
riyazdf
approved these changes
Aug 24, 2018
| // Repository represents the set of options that must be supported over a TUF repo. | ||
| // ReadOnly represents the set of options that must be supported over a TUF repo for | ||
| // reading | ||
| type ReadOnly interface { |
…loader that may be easier to use for clients that do not need any publishing capabilities. Signed-off-by: Ying Li <[email protected]>
cyli
force-pushed
the
notary-client-refactor
branch
from
4096cf4 to
c6360bf
Compare
|
🎉 /cc @tiborvass @anusha-ragunathan this one's merged |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor the notary client library to have a separate reader and loader that may be easier to use for clients that do not need any publishing capabilities.
Originally proposed something similar in #968 - this just makes it easier to set up a downloading client that can read to an in-memory cache, or just something that will load from cache without having to provide a CryptoService, etc.
The plan is, if this PR passes design and code review, for a new PR converting all the
client_update_teststo just use theLoadTUFRepofunction and an in-memory cache.cc @justincormack