You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Daniel Estévez <no...@github.com> on 2018/12/13 21:22:49 UTC
[jclouds/jclouds] [JCLOUDS-1474] [ARM] Enable Standard SKUs for Load
Balancers and PublicIPsg (#1264)
* Adds SKU field to PublicIPAddress and LoadBalancer
* Adds LiveTests to create these new type of entities
* Modifies existing calls to have default SKU null, which translates to Basic in Azure deployments. Jclouds will keep creating these Basic SKU IPs when deploying a VM
https://issues.apache.org/jira/browse/JCLOUDS-1474
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/1264
-- Commit Summary --
* Adds SKU field to both LB and PublicIP
-- File Changes --
M providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/AzureComputeProviderMetadata.java (4)
M providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/AzureComputeServiceAdapter.java (3)
M providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/domain/LoadBalancer.java (8)
A providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/domain/LoadBalancerSKU.java (44)
A providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/domain/PublicAddressSKU.java (44)
M providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/domain/PublicIPAddress.java (8)
M providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/features/LoadBalancerApi.java (3)
M providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/features/PublicIPAddressApi.java (7)
M providers/azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/features/LoadBalancerApiLiveTest.java (41)
M providers/azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/features/LoadBalancerApiMockTest.java (15)
M providers/azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/features/PublicIPAddressApiLiveTest.java (33)
M providers/azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/features/PublicIPAddressApiMockTest.java (20)
M providers/azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/features/VirtualNetworkApiLiveTest.java (2)
M providers/azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/features/VirtualNetworkGatewayApiLiveTest.java (2)
M providers/azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/features/VirtualNetworkGatewayConnectionApiLiveTest.java (2)
M providers/azurecompute-arm/src/test/resources/PublicIPAddressCreate.json (5)
M providers/azurecompute-arm/src/test/resources/PublicIPAddressGetInfo.json (5)
M providers/azurecompute-arm/src/test/resources/loadbalancercreate.json (5)
M providers/azurecompute-arm/src/test/resources/loadbalancerget.json (5)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/1264.patch
https://github.com/jclouds/jclouds/pull/1264.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1264
Re: [jclouds/jclouds] [JCLOUDS-1474] [ARM] Enable Standard SKUs for
Load Balancers and PublicIPsg (#1264)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> +import org.jclouds.json.SerializedNames;
+
+import com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class PublicAddressSKU {
+
+ public static enum PublicIPAddressSkuName {
+ Basic, Standard, Unrecognized;
+
+ public static PublicIPAddressSkuName fromValue(final String text) {
+ return (PublicIPAddressSkuName) GetEnumValue.fromValueOrDefault(text, PublicIPAddressSkuName.Unrecognized);
+ }
+ }
+
+ @Nullable
Same as before. Will change
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1264#discussion_r242172648
Re: [jclouds/jclouds] [JCLOUDS-1474] [ARM] Enable Standard SKUs for
Load Balancers and PublicIPsg (#1264)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> +import org.jclouds.json.SerializedNames;
+
+import com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class LoadBalancerSKU {
+
+ public static enum LoadBalancerSKUName {
+ Basic, Standard, Unrecognized;
+
+ public static LoadBalancerSKUName fromValue(final String text) {
+ return (LoadBalancerSKUName) GetEnumValue.fromValueOrDefault(text, LoadBalancerSKUName.Unrecognized);
+ }
+ }
+
+ @Nullable
I don't think this is nullable. Can the only field in the object be null?
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.azurecompute.arm.domain;
+
+import org.jclouds.azurecompute.arm.util.GetEnumValue;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+import com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class LoadBalancerSKU {
+
+ public static enum LoadBalancerSKUName {
Just call it `Name` and reference it as `LoadBalancerSKU.Name`.
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.azurecompute.arm.domain;
+
+import org.jclouds.azurecompute.arm.util.GetEnumValue;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+import com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class PublicAddressSKU {
+
+ public static enum PublicIPAddressSkuName {
Same naming pattern as above.
> +import org.jclouds.json.SerializedNames;
+
+import com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class PublicAddressSKU {
+
+ public static enum PublicIPAddressSkuName {
+ Basic, Standard, Unrecognized;
+
+ public static PublicIPAddressSkuName fromValue(final String text) {
+ return (PublicIPAddressSkuName) GetEnumValue.fromValueOrDefault(text, PublicIPAddressSkuName.Unrecognized);
+ }
+ }
+
+ @Nullable
Probably this is not nullable.
> @@ -68,7 +69,7 @@
@MapBinder(BindToJsonPayload.class)
LoadBalancer createOrUpdate(@PathParam("loadbalancername") String lbName,
@PayloadParam("location") String location, @Nullable @PayloadParam("tags") Map<String, String> tags,
- @PayloadParam("properties") LoadBalancerProperties properties);
+ @PayloadParam("properties") LoadBalancerProperties properties, @Nullable @PayloadParam("sku") LoadBalancerSKU sku);
Leave the properties parameter at the end. That is more aligned with the style of all other APIs and methods.
> @@ -66,9 +67,9 @@
@MapBinder(BindToJsonPayload.class)
@PUT
PublicIPAddress createOrUpdate(@PathParam("publicipaddressname") String publicipaddressname,
- @PayloadParam("location") String location,
- @Nullable @PayloadParam("tags") Map<String, String> tags,
- @PayloadParam("properties") PublicIPAddressProperties properties);
+ @PayloadParam("location") String location, @Nullable @PayloadParam("tags") Map<String, String> tags,
+ @PayloadParam("properties") PublicIPAddressProperties properties,
+ @Nullable @PayloadParam("sku") PublicAddressSKU sku);
Same comment, keep the properties object at the end.
> @@ -0,0 +1,44 @@
+/*
You probably can embed this type in the LoadBalancer type. It only makes sense inside the LB and naming is more meaningful as you just do `LoadBalancer.SKU`. Up to you.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1264#pullrequestreview-185425295
Re: [jclouds/jclouds] [JCLOUDS-1474] [ARM] Enable Standard SKUs for
Load Balancers and PublicIPsg (#1264)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> @@ -0,0 +1,44 @@
+/*
Same concern as before keeping the original MS names, but embedding it seems a good idea.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1264#discussion_r242170495
Re: [jclouds/jclouds] [JCLOUDS-1474] [ARM] Enable Standard SKUs for
Load Balancers and PublicIPsg (#1264)
Posted by Ignasi Barrera <no...@github.com>.
nacx approved this pull request.
>
@SerializedNames({ "id", "name", "location", "etag", "tags", "properties", "sku"})
public static LoadBalancer create(String id, final String name, final String location, final String etag,
- final Map<String, String> tags, final LoadBalancerProperties properties, final LoadBalancerSKU sku) {
+ final Map<String, String> tags, final LoadBalancerProperties properties, final SKU sku) {
Keep properties as the last parameter, for consistency.
>
@SerializedNames({ "name", "id", "etag", "location", "tags", "properties", "sku" })
public static PublicIPAddress create(String name, String id, String etag, String location, Map<String, String> tags,
- PublicIPAddressProperties properties, PublicAddressSKU sku) {
+ PublicIPAddressProperties properties, SKU sku) {
Keep properties at the end.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1264#pullrequestreview-187122482
Re: [jclouds/jclouds] [JCLOUDS-1474] [ARM] Enable Standard SKUs for
Load Balancers and PublicIPsg (#1264)
Posted by Daniel Estévez <no...@github.com>.
Thanks for the review @nacx !
Finally i added all SKU entities as inner classes so it's easier to reference them in code.
Also both SKUs have a `SKUName` field similar to what we do at `AvailabilitySet`
I also noticed there is a generic `SKU` entity that should be under Vault and a `VirtualMachineScaleSKU` with the same problem as in this PR's original commit
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1264#issuecomment-447976090
Re: [jclouds/jclouds] [JCLOUDS-1474] [ARM] Enable Standard SKUs for
Load Balancers and PublicIPsg (#1264)
Posted by Daniel Estévez <no...@github.com>.
rebuild please
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1264#issuecomment-448626652
Re: [jclouds/jclouds] [JCLOUDS-1474] [ARM] Enable Standard SKUs for
Load Balancers and PublicIPsg (#1264)
Posted by Daniel Estévez <no...@github.com>.
Closed #1264.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1264#event-2047757965
Re: [jclouds/jclouds] [JCLOUDS-1474] [ARM] Enable Standard SKUs for
Load Balancers and PublicIPsg (#1264)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> +import org.jclouds.json.SerializedNames;
+
+import com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class LoadBalancerSKU {
+
+ public static enum LoadBalancerSKUName {
+ Basic, Standard, Unrecognized;
+
+ public static LoadBalancerSKUName fromValue(final String text) {
+ return (LoadBalancerSKUName) GetEnumValue.fromValueOrDefault(text, LoadBalancerSKUName.Unrecognized);
+ }
+ }
+
+ @Nullable
You're right, the whole `LoadBalancerSKU` can be null and will default to `Basic` but this field is mandatory. Will change
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1264#discussion_r242172550
Re: [jclouds/jclouds] [JCLOUDS-1474] [ARM] Enable Standard SKUs for
Load Balancers and PublicIPsg (#1264)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.azurecompute.arm.domain;
+
+import org.jclouds.azurecompute.arm.util.GetEnumValue;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+import com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class LoadBalancerSKU {
+
+ public static enum LoadBalancerSKUName {
I like `Name` better but i was trying to respect the original MS names for this entity since we are mapping their domain objects. Does it make sense?
https://docs.microsoft.com/en-us/rest/api/load-balancer/loadbalancers/get#loadbalancerskuname
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1264#discussion_r242172576
Re: [jclouds/jclouds] [JCLOUDS-1474] [ARM] Enable Standard SKUs for
Load Balancers and PublicIPsg (#1264)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.azurecompute.arm.domain;
+
+import org.jclouds.azurecompute.arm.util.GetEnumValue;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+import com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class PublicAddressSKU {
+
+ public static enum PublicIPAddressSkuName {
same concern!
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1264#discussion_r242172632
Re: [jclouds/jclouds] [JCLOUDS-1474] [ARM] Enable Standard SKUs for
Load Balancers and PublicIPsg (#1264)
Posted by Daniel Estévez <no...@github.com>.
@danielestevez pushed 2 commits.
93356e726b3872c179310738439c4ddfcf3a3a66 Changes order in parameters to keep properties as last parameter
888ad76b12ec86c180a4ad5d6054de556f9506c3 Moves LoadBalancer and PublicAddress to new package with SKU as inner
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1264/files/0fefbfc7e9e32228136d2b3f4f1fc1a094dc44fc..888ad76b12ec86c180a4ad5d6054de556f9506c3
Re: [jclouds/jclouds] [JCLOUDS-1474] [ARM] Enable Standard SKUs for
Load Balancers and PublicIPsg (#1264)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.azurecompute.arm.domain;
+
+import org.jclouds.azurecompute.arm.util.GetEnumValue;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+import com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class LoadBalancerSKU {
+
+ public static enum LoadBalancerSKUName {
Besides the 'respect original MS domain names', keeping the name makes it easier to find the entity documentation.
But as i said, we could keep our own simpler and more readable field names. Just not sure what's better
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1264#discussion_r242173517
Re: [jclouds/jclouds] [JCLOUDS-1474] [ARM] Enable Standard SKUs for
Load Balancers and PublicIPsg (#1264)
Posted by Daniel Estévez <no...@github.com>.
Merged to [2.1.x](http://git-wip-us.apache.org/repos/asf/jclouds/commit/3d308560) and [master](http://git-wip-us.apache.org/repos/asf/jclouds/commit/d621edd7)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1264#issuecomment-450433948
Re: [jclouds/jclouds] [JCLOUDS-1474] [ARM] Enable Standard SKUs for
Load Balancers and PublicIPsg (#1264)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> @@ -66,9 +67,9 @@
@MapBinder(BindToJsonPayload.class)
@PUT
PublicIPAddress createOrUpdate(@PathParam("publicipaddressname") String publicipaddressname,
- @PayloadParam("location") String location,
- @Nullable @PayloadParam("tags") Map<String, String> tags,
- @PayloadParam("properties") PublicIPAddressProperties properties);
+ @PayloadParam("location") String location, @Nullable @PayloadParam("tags") Map<String, String> tags,
+ @PayloadParam("properties") PublicIPAddressProperties properties,
+ @Nullable @PayloadParam("sku") PublicAddressSKU sku);
sure
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1264#discussion_r242170537
Re: [jclouds/jclouds] [JCLOUDS-1474] [ARM] Enable Standard SKUs for
Load Balancers and PublicIPsg (#1264)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> @@ -68,7 +69,7 @@
@MapBinder(BindToJsonPayload.class)
LoadBalancer createOrUpdate(@PathParam("loadbalancername") String lbName,
@PayloadParam("location") String location, @Nullable @PayloadParam("tags") Map<String, String> tags,
- @PayloadParam("properties") LoadBalancerProperties properties);
+ @PayloadParam("properties") LoadBalancerProperties properties, @Nullable @PayloadParam("sku") LoadBalancerSKU sku);
Fine!
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1264#discussion_r242172745
Re: [jclouds/jclouds] [JCLOUDS-1474] [ARM] Enable Standard SKUs for
Load Balancers and PublicIPsg (#1264)
Posted by Daniel Estévez <no...@github.com>.
@danielestevez pushed 1 commit.
ce752f83455c19385bc938e0d50c85979b67f5f2 Reorder parameters sku and properties
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1264/files/888ad76b12ec86c180a4ad5d6054de556f9506c3..ce752f83455c19385bc938e0d50c85979b67f5f2