[Ready For Review] Virtual machine scale sets #1071
Conversation
…the primary load balancers
|
Can one of the admins verify this patch? |
|
Runtime changes detected. pull request created. CI running: |
…ing VMSS in an invalid state (bug in crp)
…et facing load balancer
anuchandy
changed the title
| * virtual machine's primary network interface in the scale set. | ||
| * | ||
| */ | ||
| interface WithSubnet { |
There was a problem hiding this comment.
instead of selecting the network first and then the subnet name, you could just do in in one call: see Frontend's withExistingSubnet
| * The stage of a virtual machine scale set update containing inputs for the resource to be updated | ||
| * (via {@link WithApplicable#apply()}). | ||
| */ | ||
| interface WithApplicable extends |
There was a problem hiding this comment.
Not sure about WithApplicable as the naming here - i think what was meant was WithApply
But i'm also wondering why this interface is needed - i just point have optional methods return Update
There was a problem hiding this comment.
Right, i mean WithApply.
The reason for this interface is - i want to maintain a context in the initial stages of update. i.e. during update i want show withInternal|internetFacingLoadBalancer and if user choose it then give him an option to specify the backends and natpool from the load balancer.
| } | ||
|
|
||
| @Override | ||
| public VirtualMachineScaleSetImpl disableVmAgent() { |
There was a problem hiding this comment.
the naming convention we generally follow for these, especially at the top level resource, is to start with 'with' to help with the disoverability. So the consistent and intuitive naming here would simply be withVmAgent()/withoutVmAgent()
| } | ||
|
|
||
| @Override | ||
| public VirtualMachineScaleSetImpl disableAutoUpdate() { |
There was a problem hiding this comment.
along similar lines to the previous comment, couldn't this be simply withAutoUpdate()/withoutAutoUpdate()?
| // Actions | ||
| // | ||
| /** | ||
| * @return available skus for the virtual machine scale set including the minimum and maximum vm instances |
There was a problem hiding this comment.
Javadoc convention: no period at the end of the @return and other such parts. Only the description uses proper casing and periods, because those are intended to be full sentences.
|
@martinsawicki please take a look, addressed your comments, thanks! |
|
@azuresdkci retest this please |
| * | ||
| * @throws IOException the IO exception | ||
| */ | ||
| Map<String, Backend> primaryInternetFacingLoadBalancerBackEnds() throws IOException; |
| * | ||
| * @throws IOException the IO exception | ||
| */ | ||
| Map<String, Backend> primaryInternetFacingLoadBalancerBackEnds() throws IOException; |
| * | ||
| * @throws IOException the IO exception | ||
| */ | ||
| Map<String, Backend> primaryInternalLoadBalancerBackEnds() throws IOException; |
| } | ||
|
|
||
| /** | ||
| * The stage of the virtual machine scale set definition allowing to specify Sku for the virtual machines. |
There was a problem hiding this comment.
"specify Sku" --> "specify the SKU" (it's an acronym)
| */ | ||
| interface WithSku { | ||
| /** | ||
| * Specifies sku for the virtual machines in the scale set. |
| WithNetworkSubnet withSku(VirtualMachineScaleSetSkuTypes skuType); | ||
|
|
||
| /** | ||
| * Specifies sku for the virtual machines in the scale set. |
|
|
||
| /** | ||
| * The stage of the virtual machine scale set definition allowing to specify a public load balancer for | ||
| * the primary network interface of the scale set virtual machines. |
There was a problem hiding this comment.
may be cleaner to say "[...] for the primary network interfaces of the virtual machines in the scale set."
| */ | ||
| interface WithPrimaryInternetFacingLoadBalancer { | ||
| /** | ||
| * Specify the public load balancer where it's backends and/or NAT pools can be assigned to the primary network |
There was a problem hiding this comment.
maybe better: "Specify the Internet-facing load balancer whose backends and/or NAT pools can be assigned to the primary network interfaces of the virtual machines in the scale set."
| * in the scale set. | ||
| * <p> | ||
| * A primary internal load balancer associated with the primary network interfaces of the scale set | ||
| * virtual machine will be also belongs to this network |
| Network primaryNetwork() throws IOException; | ||
|
|
||
| /** | ||
| * @return the internet facing load balancer associated with the primary network interface of |
| LoadBalancer primaryInternetFacingLoadBalancer() throws IOException; | ||
|
|
||
| /** | ||
| * @return the internet facing load balancer's backends associated with the primary network interface |
| Map<String, Backend> primaryInternetFacingLoadBalancerBackEnds() throws IOException; | ||
|
|
||
| /** | ||
| * @return the internet facing load balancer's inbound NAT pool associated with the primary network interface |
There was a problem hiding this comment.
"Internet-facing".
I won't comment on these anymore -- it'd be good to just search/replace "internet facing" with "Internet-facing"
| Map<String, InboundNatPool> primaryInternalLoadBalancerInboundNatPools() throws IOException; | ||
|
|
||
| /** | ||
| * @return the list of ids of public Ip addresses associated with the primary internet facing load balancer |
There was a problem hiding this comment.
"the list of IDs of the public IP addresses [...]"
| List<String> primaryPublicIpAddressIds() throws IOException; | ||
|
|
||
| /** | ||
| * @return the url to storage containers that stores vhds of virtual machines in the scale set |
| VirtualMachineScaleSetNetworkProfile networkProfile(); | ||
|
|
||
| /** | ||
| * @return the extensions attached to the Virtual Machines in the scale set |
There was a problem hiding this comment.
"virtual machines" (capitalization)
| /** | ||
| * Specifies sku for the virtual machines in the scale set. | ||
| * | ||
| * @param sku a sku from the list of available sizes for the virtual machines in this scale set |
There was a problem hiding this comment.
" a SKU". Let's just search/replace all "sku" with "SKU" in javadocs (not code)
| } | ||
|
|
||
| /** | ||
| * The stage of the virtual machine scale set definition allowing to specify virtual network subnet for the |
| * Specify the public load balancer where it's backends and/or NAT pools can be assigned to the primary network | ||
| * interface of the scale set virtual machines. | ||
| * <p> | ||
| * By default all the backend and inbound NAT pool of the load balancer will be associated with the primary |
There was a problem hiding this comment.
"backends and inbound NAT pools"
| WithPrimaryInternetFacingLoadBalancerBackendOrNatPool withPrimaryInternetFacingLoadBalancer(LoadBalancer loadBalancer); | ||
|
|
||
| /** | ||
| * Specifies that no public load balancer needs to be associated with virtual machine scale set. |
There was a problem hiding this comment.
"[...] with the virtual machine scale set."
|
|
||
| /** | ||
| * The stage of the virtual machine scale set definition allowing to specify an internal load balancer for | ||
| * the primary network interface of the scale set virtual machines. |
There was a problem hiding this comment.
maybe cleaner: "[...] of the virtual machines in the scale set."
| */ | ||
| interface WithPrimaryInternalLoadBalancer { | ||
| /** | ||
| * Specify the internal load balancer where it's backends and/or NAT pools can be assigned to the primary network |
There was a problem hiding this comment.
overall, i just focused more on the Javadoc content, but it looks like it will be more efficient to just do a big Javadoc scrub across the entire SDK's API surface, as there are a number of stylistic conventions that are appropriate, so my Javadoc input is not the highest pri.
merging since code review has been addressed. there are just some naming corrections (Backend not BackEnd) which we can followup up with aftet the merge, but the javadoc scrub will need to happen as a more focused effort.
merging since code review has been addressed. there are just some naming corrections (Backend not BackEnd) which we can followup up with aftet the merge, but the javadoc scrub will need to happen as a more focused effort.
No description provided.