-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: cache client control plane operations #44
Conversation
@@ -12,7 +12,7 @@ void main() async { | |||
|
|||
var topicClient = TopicClient( | |||
CredentialProvider.fromEnvironmentVariable("MOMENTO_API_KEY"), | |||
Mobile.latest()); | |||
MobileTopicConfiguration.latest()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
names clashed when both topic and cache configs were exported from the top-level lib/client_sdk_dart.dart
file
CacheConfiguration configuration, Duration defaultTtl) { | ||
_dataClient = DataClient(credentialProvider, configuration, defaultTtl); | ||
_controlClient = ControlClient(credentialProvider, configuration); | ||
// _logger.level = determineLoggerLevel(configuration.logLevel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue captured in #36
_logger.finest("initializing cache client"); | ||
} | ||
|
||
@override | ||
Future<CreateCacheResponse> createCache(String cacheName) { | ||
// TODO: add validators |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will be added in #43 which will also round out all the basic cache operations
class CreateCacheSuccess implements CreateCacheResponse {} | ||
|
||
/// Indicates that the cache already exists, so there was nothing to do. | ||
class CreateCacheAlreadyExists implements CreateCacheResponse {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you feel about the name AlreadyExists
instead of CreateCacheAlreadyExists
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's definitely more concise, I can change that 👍
}); | ||
|
||
group('control plane operations', () { | ||
test('arguments are validated', () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like putting these placeholder tests here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addresses #25 and creates integration test setup and teardown functions