-
Notifications
You must be signed in to change notification settings - Fork 4
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
chore: remove next token from list_caches and list_signing_keys #173
Conversation
Remove the next_token argument from list_caches and list_signing_keys. It isn't used by the back end and clutters the function signature. Add a delete_cache to an integration test that was missing one. The tests may still leak caches if the tests fail before getting to the delete cache statement at the end. More comprehensive protection will follow.
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.
LGTM just one question about protobuf defaults
let request = Request::new(ListCachesRequest { | ||
next_token: next_token.unwrap_or_default(), | ||
next_token: String::new(), |
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.
Can we just omit this line? Or do we need to explicitly pass a default?
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.
The rust struct requires a string here, and it's not Optional, so we have to pass something :) idk if there is some kind of built-in Tonic "default" thing to use here as oppose to String.new though. @kvcache ?
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.
We're just using an empty string in the other SDKs.
let request = Request::new(ListSigningKeysRequest { | ||
next_token: next_token.unwrap_or_default().to_string(), | ||
next_token: String::new(), |
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.
Same question here re: default
let request = Request::new(ListCachesRequest { | ||
next_token: next_token.unwrap_or_default(), | ||
next_token: String::new(), |
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.
The rust struct requires a string here, and it's not Optional, so we have to pass something :) idk if there is some kind of built-in Tonic "default" thing to use here as oppose to String.new though. @kvcache ?
Remove the next_token argument from list_caches and list_signing_keys. It isn't used by the back end and clutters the function signature.
Add a delete_cache to an integration test that was missing one. The tests may still leak caches if the tests fail before getting to the delete cache statement at the end. More comprehensive protection will follow.