-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[Ready for review] Updated batch to support batch account creation with fluent pattern. #1066
Conversation
Can one of the admins verify this patch? |
1 similar comment
Can one of the admins verify this patch? |
8c6854e
to
65b1f64
Compare
7c51926
to
12ccba3
Compare
@azuresdkci add to whitelist |
* @throws CloudException exception thrown from REST call | ||
* @throws IOException exception thrown from serialization/deserialization | ||
*/ | ||
BatchAccountKeys keys() throws CloudException, IOException; |
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.
it looks like this is a separate call to Azure. If that's the case, we're starting to follow the convention to use "get" as the prefix on such methods, to indicate the info is not a property but a separate method that calls Azure. So in this case, we could call this getKeys()
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.
Changed it to set of 2 functions keys and refreshKeys which are similar to Storage key apis.
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'm guessing you mean .keys() would quickly return the locally cached keys and refreshKeys() would make th actual call (and return the keys as well, right?) that makes sense to me as well.
I assume this will be added to this PR (not seeing a new commit yet)
12ccba3
to
1bf5330
Compare
* | ||
* @return the stage representing updatable batch account definition | ||
*/ | ||
Update removeStorageAccount(); |
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 convention we use for removing things is "without..." so it'd be withoutStorageAccount()
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.
done.
1bf5330
to
3ffdf38
Compare
[Ready for review] Updated batch to support batch account creation with fluent pattern.
[Ready for review] Updated batch to support batch account creation with fluent pattern.
Updated batch to support batch account creation with fluent pattern.