Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
merged at (master)[http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/89cb6f0f] -- 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-labs/pull/308#issuecomment-248317509
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
Closed #308. -- 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-labs/pull/308#event-795734646
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
@andreaturli pushed 1 commit. 07007d9 refactor logback-test.xml -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds-labs/pull/308/files/8f403aa5cc1c711496a670c3fbf3e03eadca969e..07007d994a33b1cc3c62a6294e8760141014758e
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
@andreaturli pushed 2 commits. 12f49f2 add parser module 8f403aa separate the DeploymentToVMDeployment to a function -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds-labs/pull/308/files/aaae68457526b198a2b30d7f648b4ef0b46ec359..8f403aa5cc1c711496a670c3fbf3e03eadca969e
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
nacx commented on this pull request.
> @@ -46,9 +44,9 @@ protected void bindErrorHandlers() {
@Override
protected void installLocations() {
super.installLocations();
- bind(ImplicitLocationSupplier.class).
- to(OnlyLocationOrFirstRegionOptionallyMatchingRegionId.class).
I've had a deeper look at this and I agree with 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-labs/pull/308
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
nacx commented on this pull request.
> @@ -28,6 +30,7 @@ public static Properties defaultProperties(Properties
> properties) {
properties.put("oauth.credential", "password");
properties.put("oauth.endpoint",
"https://login.microsoftonline.com/oauth2/token;);
properties.put(CREDENTIAL_TYPE, CLIENT_CREDENTIALS_SECRET.toString());
+ properties.put(PROPERTY_REGIONS, "northeurope,eastus");
Ok, dismiss this comment. I was under the assumption that the changes in the
LocationToLocation class were made to support the ZONE approach, but I see
they're not related. What is the purpose of that change in the location id ?
--
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-labs/pull/308
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
nacx commented on this pull request.
> -vmLocations.addAll(m.locations());
-break;
- }
- }
-
- Iterable result = Iterables.filter(locations, new
Predicate() {
- @Override
- public boolean apply(Location input) {
-return vmLocations.contains(input.displayName());
- }
- });
-
- return result;
+ private Iterable findWhiteListOfRegions() {
+ if
(providerMetadata.getDefaultProperties().get(LocationConstants.PROPERTY_REGIONS)
== null) return null;
+ return Splitter.on(",").trimResults().split((CharSequence)
providerMetadata.getDefaultProperties().get(LocationConstants.PROPERTY_REGIONS));
Is this cast to CharSequence needed?
--
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-labs/pull/308#pullrequestreview-564604
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
andreaturli commented on this pull request.
> @@ -28,6 +30,7 @@ public static Properties defaultProperties(Properties
> properties) {
properties.put("oauth.credential", "password");
properties.put("oauth.endpoint",
"https://login.microsoftonline.com/oauth2/token;);
properties.put(CREDENTIAL_TYPE, CLIENT_CREDENTIALS_SECRET.toString());
+ properties.put(PROPERTY_REGIONS, "northeurope,eastus");
Not sure I understand this comment?
--
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-labs/pull/308
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
andreaturli commented on this pull request.
> @Inject
AzureComputeServiceAdapter(final AzureComputeApi api, final
AzureComputeConstants azureComputeConstants,
- CleanupResources cleanupResources) {
+ CleanupResources cleanupResources,
ProviderMetadata providerMetadata) {
yes, but it doesn't work as well
--
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-labs/pull/308
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
nacx requested changes on this pull request.
Thanks @andreaturli! I've added the comments for the things that I think we
have to change.
> @Inject
AzureComputeServiceAdapter(final AzureComputeApi api, final
AzureComputeConstants azureComputeConstants,
- CleanupResources cleanupResources) {
+ CleanupResources cleanupResources,
ProviderMetadata providerMetadata) {
Have you tried this?
> @@ -300,28 +288,43 @@ public VMImage getImage(final String id) {
@Override
public Iterable listLocations() {
+ final Iterable whiteListZoneName = findWhiteListOfRegions();
Rename to `whiteListedRegionNames`?
> -vmLocations.addAll(m.locations());
-break;
- }
- }
-
- Iterable result = Iterables.filter(locations, new
Predicate() {
- @Override
- public boolean apply(Location input) {
-return vmLocations.contains(input.displayName());
- }
- });
-
- return result;
+ private Iterable findWhiteListOfRegions() {
+ if
(providerMetadata.getDefaultProperties().get(LocationConstants.PROPERTY_REGIONS)
== null) return null;
+ return Splitter.on(",").trimResults().split((CharSequence)
providerMetadata.getDefaultProperties().get(LocationConstants.PROPERTY_REGIONS));
}
private String getResourceGroupFromId(String id) {
If we have a global property that filters locations, we should also filter the
`listNodes` operation.
How should the `getImage` and `getNode` behave when invoked with a resource
that is from another location?
>if (region != null) {
builder.iso3166Codes(ImmutableSet.of(region.iso3166Code()));
}
-
return builder.build();
}
Now that we agree we shouldn't change REGION to ZONE, let's discard the changes
in this class.
> @@ -46,9 +44,9 @@ protected void bindErrorHandlers() {
@Override
protected void installLocations() {
super.installLocations();
- bind(ImplicitLocationSupplier.class).
- to(OnlyLocationOrFirstRegionOptionallyMatchingRegionId.class).
This was working before. If you discard the changes in the location
transformation function, can't you just discard the changes in this class too?
>
import static com.google.common.base.Preconditions.checkNotNull;
-import org.jclouds.logging.slf4j.config.SLF4JLoggingModule;
-import org.jclouds.logging.config.LoggingModule;
-
+import static
org.jclouds.azurecompute.arm.config.AzureComputeProperties.RESOURCE_GROUP_NAME;
+import static
org.jclouds.compute.config.ComputeServiceProperties.TIMEOUT_SCRIPT_COMPLETE;
+import static
org.jclouds.compute.config.ComputeServiceProperties.TIMEOUT_NODE_RUNNING;
+import static
org.jclouds.compute.config.ComputeServiceProperties.TIMEOUT_PORT_OPEN;
+import static
org.jclouds.compute.config.ComputeServiceProperties.TIMEOUT_NODE_TERMINATED;
+import static
org.jclouds.compute.config.ComputeServiceProperties.TIMEOUT_NODE_SUSPENDED;
/**
* Live tests for the {@link org.jclouds.compute.ComputeService} integration.
Just remove this from the change-set
> @@ -28,6 +30,7 @@ public static Properties defaultProperties(Properties
> properties) {
properties.put("oauth.credential", "password");
properties.put("oauth.endpoint",
"https://login.microsoftonline.com/oauth2/token;);
properties.put(CREDENTIAL_TYPE, CLIENT_CREDENTIALS_SECRET.toString());
+ properties.put(PROPERTY_REGIONS, "northeurope,eastus");
When rolling back the location transformation function you'll probably have to
change these values. They also appear in the README as an example, so bear in
mind to change that too.
--
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-labs/pull/308#pullrequestreview-520038
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
> @@ -46,9 +44,9 @@ protected void bindErrorHandlers() {
> @Override
> protected void installLocations() {
>super.installLocations();
> - bind(ImplicitLocationSupplier.class).
> - to(OnlyLocationOrFirstRegionOptionallyMatchingRegionId.class).
I think `FirstRegion` as `ImplicitLocationSupplier` is a better choice here
--
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-labs/pull/308/files/961db5c145342971726063e25776e0011ab8ed36#r78753512
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
@andreaturli pushed 1 commit. aaae684 revert ZONES to REGIONS -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds-labs/pull/308/files/961db5c145342971726063e25776e0011ab8ed36..aaae68457526b198a2b30d7f648b4ef0b46ec359
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
> @@ -46,9 +44,9 @@ protected void bindErrorHandlers() {
> @Override
> protected void installLocations() {
>super.installLocations();
> - bind(ImplicitLocationSupplier.class).
> - to(OnlyLocationOrFirstRegionOptionallyMatchingRegionId.class).
@nacx I'm having issues in restoring
`OnlyLocationOrFirstRegionOptionallyMatchingRegionId` as it fall-back to
`providerUri`which is always the same for each region. Wrong supplier?
--
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-labs/pull/308/files/961db5c145342971726063e25776e0011ab8ed36#r78728598
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
ok thanks, I guess this one confused me https://github.com/jclouds/jclouds-labs/pull/308#issuecomment-239163128 :) -- 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-labs/pull/308#issuecomment-246973355
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
I don't think so. The last comments have not been addressed. We agreed that the change form region to zone was not good, so all that stuff needs to be reverted. -- 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-labs/pull/308#issuecomment-246971336
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
@nacx good to merge? -- 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-labs/pull/308#issuecomment-246959604
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
> @Inject
> AzureComputeServiceAdapter(final AzureComputeApi api, final
> AzureComputeConstants azureComputeConstants,
> - CleanupResources cleanupResources) {
> + CleanupResources cleanupResources,
> ProviderMetadata providerMetadata) {
Can you try removing it from the constructor and put the annotation in the
class variable together with an `@Inject(optional = true)`?
--
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-labs/pull/308/files/961db5c145342971726063e25776e0011ab8ed36#r74608252
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
> @Inject
> AzureComputeServiceAdapter(final AzureComputeApi api, final
> AzureComputeConstants azureComputeConstants,
> - CleanupResources cleanupResources) {
> + CleanupResources cleanupResources,
> ProviderMetadata providerMetadata) {
According to https://github.com/google/guice/wiki/UseNullable I've also tried
javax.annotation.Nullable which requires
```
com.google.code.findbugs
jsr305
```
but no luck as well
--
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-labs/pull/308/files/961db5c145342971726063e25776e0011ab8ed36#r74560449
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
> @Inject
> AzureComputeServiceAdapter(final AzureComputeApi api, final
> AzureComputeConstants azureComputeConstants,
> - CleanupResources cleanupResources) {
> + CleanupResources cleanupResources,
> ProviderMetadata providerMetadata) {
I think in this way the PROPERTY_ZONES becomes mandatory (otherwise Guice is
not happy)
Is it what we want?
--
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-labs/pull/308/files/961db5c145342971726063e25776e0011ab8ed36#r74551457
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
> @@ -28,6 +30,7 @@ public static Properties defaultProperties(Properties
> properties) {
> properties.put("oauth.credential", "password");
> properties.put("oauth.endpoint",
> "https://login.microsoftonline.com/oauth2/token;);
> properties.put(CREDENTIAL_TYPE, CLIENT_CREDENTIALS_SECRET.toString());
> + properties.put(PROPERTY_ZONES, "northeurope,eastus");
```
CENTRAL_US("Central US", "US-IA"),
EAST_US("East US", "US-VA"),
EAST_US_2("East US 2", "US-VA"),
US_GOV_IOWA("US Gov Iowa", "US-IA"),
US_GOV_VIRGINIA("US Gov Virginia", "US-VA"),
NORTH_CENTRAL_US("North Central US", "US-IL"),
SOUTH_CENTRAL_US("South Central US", "US-TX"),
WEST_US("West US", "US-CA"),
NORTH_EUROPE("North Europe", "IE"),
WEST_EUROPE("West Europe", "NL"),
EAST_ASIA("East Asia", "HK"),
SOUTH_EAST_ASIA("Southeast Asia", "SG"),
JAPAN_EAST("Japan East", "JP-11"),
JAPAN_WEST("Japan West", "JP-27"),
BRAZIL_SOUTH("Brazil South", "BR"),
AUSTRALIA_EAST("Australia East", "AU-NSW"),
AUSTRALIA_SOUTH_EAST("Australia Southeast", "AU-VIC"),
INDIA_CENTRAL("Central India", "IN-GA"),
INDIA_SOUTH("South India", "IN-TN"),
INDIA_WEST("West India", "IN-MH"),
CHINA_EAST("China East", "CN-SH"),
CHINA_NORTH("China North", "CN-BJ"),
CANADA_CENTRAL("Canada Central", "CA-ON"),
CANADA_EAST("Canada East", "CA-QC");
```
--
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-labs/pull/308/files/961db5c145342971726063e25776e0011ab8ed36#r74551789
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
> @Inject
> AzureComputeServiceAdapter(final AzureComputeApi api, final
> AzureComputeConstants azureComputeConstants,
> - CleanupResources cleanupResources) {
> + CleanupResources cleanupResources,
> ProviderMetadata providerMetadata) {
yes
--
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-labs/pull/308/files/961db5c145342971726063e25776e0011ab8ed36#r74558750
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
> @Inject
> AzureComputeServiceAdapter(final AzureComputeApi api, final
> AzureComputeConstants azureComputeConstants,
> - CleanupResources cleanupResources) {
> + CleanupResources cleanupResources,
> ProviderMetadata providerMetadata) {
Does it complain too if you annotate it `@Nullable` as suggested?
--
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-labs/pull/308/files/961db5c145342971726063e25776e0011ab8ed36#r74552021
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
> @@ -28,6 +30,7 @@ public static Properties defaultProperties(Properties
> properties) {
> properties.put("oauth.credential", "password");
> properties.put("oauth.endpoint",
> "https://login.microsoftonline.com/oauth2/token;);
> properties.put(CREDENTIAL_TYPE, CLIENT_CREDENTIALS_SECRET.toString());
> + properties.put(PROPERTY_ZONES, "northeurope,eastus");
Yes. If they are regions, then keep the region naming unchanged.
--
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-labs/pull/308/files/961db5c145342971726063e25776e0011ab8ed36#r74441733
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
> @@ -28,6 +30,7 @@ public static Properties defaultProperties(Properties
> properties) {
> properties.put("oauth.credential", "password");
> properties.put("oauth.endpoint",
> "https://login.microsoftonline.com/oauth2/token;);
> properties.put(CREDENTIAL_TYPE, CLIENT_CREDENTIALS_SECRET.toString());
> + properties.put(PROPERTY_ZONES, "northeurope,eastus");
those are the region names according to http://azure.microsoft.com/en-us/regions
--
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-labs/pull/308/files/961db5c145342971726063e25776e0011ab8ed36#r74430909
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
Thanks @andreaturli! -- 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-labs/pull/308#issuecomment-239163128
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
> @@ -28,6 +30,7 @@ public static Properties defaultProperties(Properties
> properties) {
> properties.put("oauth.credential", "password");
> properties.put("oauth.endpoint",
> "https://login.microsoftonline.com/oauth2/token;);
> properties.put(CREDENTIAL_TYPE, CLIENT_CREDENTIALS_SECRET.toString());
> + properties.put(PROPERTY_ZONES, "northeurope,eastus");
How do locations look like in Azure? These looks more like Regions? I mean,
zones in providers are usually in the form: northeurope-1, northeurope2, etc.
--
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-labs/pull/308/files/961db5c145342971726063e25776e0011ab8ed36#r74422920
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
>
> import static com.google.common.base.Preconditions.checkNotNull;
> -import org.jclouds.logging.slf4j.config.SLF4JLoggingModule;
> -import org.jclouds.logging.config.LoggingModule;
> -
> +import static
> org.jclouds.azurecompute.arm.config.AzureComputeProperties.RESOURCE_GROUP_NAME;
> +import static
> org.jclouds.compute.config.ComputeServiceProperties.TIMEOUT_SCRIPT_COMPLETE;
> +import static
> org.jclouds.compute.config.ComputeServiceProperties.TIMEOUT_NODE_RUNNING;
> +import static
> org.jclouds.compute.config.ComputeServiceProperties.TIMEOUT_PORT_OPEN;
> +import static
> org.jclouds.compute.config.ComputeServiceProperties.TIMEOUT_NODE_TERMINATED;
> +import static
> org.jclouds.compute.config.ComputeServiceProperties.TIMEOUT_NODE_SUSPENDED;
>
> /**
> * Live tests for the {@link org.jclouds.compute.ComputeService} integration.
Remove this class from the change set
--
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-labs/pull/308/files/961db5c145342971726063e25776e0011ab8ed36#r74422773
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
> @@ -47,20 +47,14 @@
> @Override
> public org.jclouds.domain.Location apply(final Location location) {
>final LocationBuilder builder = new LocationBuilder();
> - String id = location.id();
> - int index = id.lastIndexOf('/');
> - if (index > 0 && (index + 1) < id.length())
> - id = id.substring(index + 1);
> - builder.id(id);
> + builder.id(location.name());
>builder.description(location.displayName());
>builder.parent(getOnlyElement(justProvider.get()));
The parent location of the ZONE should be the REGION instead of the provider?
--
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-labs/pull/308/files/961db5c145342971726063e25776e0011ab8ed36#r74422538
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
> @Inject
> AzureComputeServiceAdapter(final AzureComputeApi api, final
> AzureComputeConstants azureComputeConstants,
> - CleanupResources cleanupResources) {
> + CleanupResources cleanupResources,
> ProviderMetadata providerMetadata) {
Better inject directly the `@Nullable @Named(LocationConstants.PROPERTY_ZONES)
String enabledZones` and split the property and assign the list in the
constructor.
--
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-labs/pull/308/files/961db5c145342971726063e25776e0011ab8ed36#r74422374
Re: [jclouds/jclouds-labs] add support for whitelisting locations (#308)
@andreaturli pushed 1 commit. 961db5c address initial comments from @nacx -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds-labs/pull/308/files/bea9005a1dbad01499584a1932c4445b64a60659..961db5c145342971726063e25776e0011ab8ed36
