You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by GitBox <gi...@apache.org> on 2020/11/11 14:26:13 UTC
[GitHub] [jclouds] davidsloan opened a new pull request #88: JCLOUDS-1557 - Azure local server support
davidsloan opened a new pull request #88:
URL: https://github.com/apache/jclouds/pull/88
Adding ability to use local azure-compatible services, eg Azurite
- Ensuring the endpoint URL is respected by SharedKeyLiteAuthentication and AzureBlobRequestSigner via the new StorageUrlDelegate
- Adding override property to enable the URL format of account key in the path. By default it respects the default Azure behaviour of the storage account name as part of the virtual host.
- Updating unit tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] gaul commented on pull request #88: JCLOUDS-1557 - Azure local server support
Posted by GitBox <gi...@apache.org>.
gaul commented on pull request #88:
URL: https://github.com/apache/jclouds/pull/88#issuecomment-739216189
@davidsloan We plan to release 2.3.0 later this month. Do you think you can finish this PR soon?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] nacx commented on a change in pull request #88: JCLOUDS-1557 - Azure local server support
Posted by GitBox <gi...@apache.org>.
nacx commented on a change in pull request #88:
URL: https://github.com/apache/jclouds/pull/88#discussion_r522421977
##########
File path: providers/azureblob/src/test/java/org/jclouds/azure/storage/util/StorageUrlDelegateTest.java
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * 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.azure.storage.util;
+
+import static org.testng.Assert.assertEquals;
+
+import java.util.Properties;
+
+import org.jclouds.ContextBuilder;
+import org.jclouds.azure.storage.reference.AzureConstants;
+import org.jclouds.logging.config.NullLoggingModule;
+import org.jclouds.rest.internal.BaseRestApiTest;
+import org.testng.annotations.Test;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.inject.Module;
+
+@Test(groups = "unit")
+public class StorageUrlDelegateTest {
+
+ private static final String ACCOUNT = "foo";
+ private static final ImmutableSet<Module> MODULES = ImmutableSet.<Module> of(new BaseRestApiTest.MockModule(),
+ new NullLoggingModule());
+
+ @Test
+ void testDefaultEndpoint() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector().getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "https://foo.blob.core.windows.net/");
+ }
+
+ @Test
+ void testCustomEndpointWithoutStorageAccountPath() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob").endpoint("http://localhost:10000")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector()
+ .getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "http://localhost:10000/");
Review comment:
It is about correctness. If there is a custom endpoint and the property has the default value, the account part never manifests in the generated storage URL. Is that correct?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] nacx commented on a change in pull request #88: JCLOUDS-1557 - Azure local server support
Posted by GitBox <gi...@apache.org>.
nacx commented on a change in pull request #88:
URL: https://github.com/apache/jclouds/pull/88#discussion_r522283581
##########
File path: providers/azureblob/src/test/java/org/jclouds/azure/storage/util/StorageUrlDelegateTest.java
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * 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.azure.storage.util;
+
+import static org.testng.Assert.assertEquals;
+
+import java.util.Properties;
+
+import org.jclouds.ContextBuilder;
+import org.jclouds.azure.storage.reference.AzureConstants;
+import org.jclouds.logging.config.NullLoggingModule;
+import org.jclouds.rest.internal.BaseRestApiTest;
+import org.testng.annotations.Test;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.inject.Module;
+
+@Test(groups = "unit")
+public class StorageUrlDelegateTest {
+
+ private static final String ACCOUNT = "foo";
+ private static final ImmutableSet<Module> MODULES = ImmutableSet.<Module> of(new BaseRestApiTest.MockModule(),
+ new NullLoggingModule());
+
+ @Test
+ void testDefaultEndpoint() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector().getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "https://foo.blob.core.windows.net/");
+ }
+
+ @Test
+ void testCustomEndpointWithoutStorageAccountPath() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob").endpoint("http://localhost:10000")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector()
+ .getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "http://localhost:10000/");
Review comment:
My point is, does it make sense to _ignore_ the account if a custom endpoint is configured? Should we just do the same as we do when `PROPERTY_AZURE_VIRTUAL_HOST_STORAGE_ACCOUNT == false` when a custom endpoint is set (regardless of the property value)?
Which leads me to think: doe sit make sense to set the property to `vhost` if we're setting a custom endpoint? because the current code does not handle that case. And if it does not make sense, do we still need the property? Can we just default to vhost, but if there is a custom endpoint, just append the account to the path?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] davidsloan commented on a change in pull request #88: JCLOUDS-1557 - Azure local server support
Posted by GitBox <gi...@apache.org>.
davidsloan commented on a change in pull request #88:
URL: https://github.com/apache/jclouds/pull/88#discussion_r522430914
##########
File path: providers/azureblob/src/test/java/org/jclouds/azure/storage/util/StorageUrlDelegateTest.java
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * 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.azure.storage.util;
+
+import static org.testng.Assert.assertEquals;
+
+import java.util.Properties;
+
+import org.jclouds.ContextBuilder;
+import org.jclouds.azure.storage.reference.AzureConstants;
+import org.jclouds.logging.config.NullLoggingModule;
+import org.jclouds.rest.internal.BaseRestApiTest;
+import org.testng.annotations.Test;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.inject.Module;
+
+@Test(groups = "unit")
+public class StorageUrlDelegateTest {
+
+ private static final String ACCOUNT = "foo";
+ private static final ImmutableSet<Module> MODULES = ImmutableSet.<Module> of(new BaseRestApiTest.MockModule(),
+ new NullLoggingModule());
+
+ @Test
+ void testDefaultEndpoint() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector().getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "https://foo.blob.core.windows.net/");
+ }
+
+ @Test
+ void testCustomEndpointWithoutStorageAccountPath() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob").endpoint("http://localhost:10000")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector()
+ .getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "http://localhost:10000/");
Review comment:
There are use cases where it is be desirable to have a custom endpoint without activating the storage-account-in-path format.
For example you set up a custom DNS name for your blob storage endpoint:
https://docs.microsoft.com/en-us/azure/storage/blobs/storage-custom-domain-name?tabs=azure-portal
This would not be running the local service but you would be using a custom endpoint.
This is not a use case that I am interested in, but just throwing it out there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] nacx commented on a change in pull request #88: JCLOUDS-1557 - Azure local server support
Posted by GitBox <gi...@apache.org>.
nacx commented on a change in pull request #88:
URL: https://github.com/apache/jclouds/pull/88#discussion_r522435377
##########
File path: providers/azureblob/src/test/java/org/jclouds/azure/storage/util/StorageUrlDelegateTest.java
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * 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.azure.storage.util;
+
+import static org.testng.Assert.assertEquals;
+
+import java.util.Properties;
+
+import org.jclouds.ContextBuilder;
+import org.jclouds.azure.storage.reference.AzureConstants;
+import org.jclouds.logging.config.NullLoggingModule;
+import org.jclouds.rest.internal.BaseRestApiTest;
+import org.testng.annotations.Test;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.inject.Module;
+
+@Test(groups = "unit")
+public class StorageUrlDelegateTest {
+
+ private static final String ACCOUNT = "foo";
+ private static final ImmutableSet<Module> MODULES = ImmutableSet.<Module> of(new BaseRestApiTest.MockModule(),
+ new NullLoggingModule());
+
+ @Test
+ void testDefaultEndpoint() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector().getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "https://foo.blob.core.windows.net/");
+ }
+
+ @Test
+ void testCustomEndpointWithoutStorageAccountPath() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob").endpoint("http://localhost:10000")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector()
+ .getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "http://localhost:10000/");
Review comment:
That makes sense, thanks for the link!
One last thing, then, if there is no custom endpoint configured and the property is set to `false`, the account will be included in the hostname, but appended to the path as well, which I guess is not correct?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] nacx commented on a change in pull request #88: JCLOUDS-1557 - Azure local server support
Posted by GitBox <gi...@apache.org>.
nacx commented on a change in pull request #88:
URL: https://github.com/apache/jclouds/pull/88#discussion_r522217861
##########
File path: providers/azureblob/src/test/java/org/jclouds/azure/storage/util/StorageUrlDelegateTest.java
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * 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.azure.storage.util;
+
+import static org.testng.Assert.assertEquals;
+
+import java.util.Properties;
+
+import org.jclouds.ContextBuilder;
+import org.jclouds.azure.storage.reference.AzureConstants;
+import org.jclouds.logging.config.NullLoggingModule;
+import org.jclouds.rest.internal.BaseRestApiTest;
+import org.testng.annotations.Test;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.inject.Module;
+
+@Test(groups = "unit")
+public class StorageUrlDelegateTest {
+
+ private static final String ACCOUNT = "foo";
+ private static final ImmutableSet<Module> MODULES = ImmutableSet.<Module> of(new BaseRestApiTest.MockModule(),
+ new NullLoggingModule());
+
+ @Test
+ void testDefaultEndpoint() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector().getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "https://foo.blob.core.windows.net/");
+ }
+
+ @Test
+ void testCustomEndpointWithoutStorageAccountPath() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob").endpoint("http://localhost:10000")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector()
+ .getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "http://localhost:10000/");
Review comment:
Why are we only setting the account name if `PROPERTY_AZURE_VIRTUAL_HOST_STORAGE_ACCOUNT == false` is false? I mean, what is the point in not adding the account name anywhere in the URI? Either if we have a custom endpoint or not, I guess we want the account name to be present _somewhere_?
##########
File path: providers/azureblob/src/main/java/org/jclouds/azure/storage/util/StorageUrlDelegate.java
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * 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.azure.storage.util;
+
+import static org.jclouds.azure.storage.reference.AzureConstants.PROPERTY_AZURE_VIRTUAL_HOST_STORAGE_ACCOUNT;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.jclouds.domain.Credentials;
+import org.jclouds.providers.ProviderMetadata;
+
+import com.google.common.base.Supplier;
+
+@Singleton
+public class StorageUrlDelegate {
+
+ private ProviderMetadata providerMetadata;
+ private Supplier<Credentials> credentialsSupplier;
+ private boolean storageAccountInsideVirtualHost;
+
+ @Inject
+ public StorageUrlDelegate(ProviderMetadata providerMetadata,
Review comment:
Instead of manually getting the endpoint from the provider metadata, you should inject the following endpoint supplier and get it from there, when needed.
```suggestion
public StorageUrlDelegate(@Provider Supplier<URI> endpointSupplier,
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] gaul commented on pull request #88: JCLOUDS-1557 - Azure local server support
Posted by GitBox <gi...@apache.org>.
gaul commented on pull request #88:
URL: https://github.com/apache/jclouds/pull/88#issuecomment-740314613
Thank you for your contribution and testing @davidsloan!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] davidsloan commented on a change in pull request #88: JCLOUDS-1557 - Azure local server support
Posted by GitBox <gi...@apache.org>.
davidsloan commented on a change in pull request #88:
URL: https://github.com/apache/jclouds/pull/88#discussion_r522242867
##########
File path: providers/azureblob/src/main/java/org/jclouds/azure/storage/util/StorageUrlDelegate.java
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * 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.azure.storage.util;
+
+import static org.jclouds.azure.storage.reference.AzureConstants.PROPERTY_AZURE_VIRTUAL_HOST_STORAGE_ACCOUNT;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.jclouds.domain.Credentials;
+import org.jclouds.providers.ProviderMetadata;
+
+import com.google.common.base.Supplier;
+
+@Singleton
+public class StorageUrlDelegate {
+
+ private ProviderMetadata providerMetadata;
+ private Supplier<Credentials> credentialsSupplier;
+ private boolean storageAccountInsideVirtualHost;
+
+ @Inject
+ public StorageUrlDelegate(ProviderMetadata providerMetadata,
Review comment:
Thanks for the tip, I've implemented the change you suggested!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] davidsloan commented on a change in pull request #88: JCLOUDS-1557 - Azure local server support
Posted by GitBox <gi...@apache.org>.
davidsloan commented on a change in pull request #88:
URL: https://github.com/apache/jclouds/pull/88#discussion_r522441065
##########
File path: providers/azureblob/src/test/java/org/jclouds/azure/storage/util/StorageUrlDelegateTest.java
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * 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.azure.storage.util;
+
+import static org.testng.Assert.assertEquals;
+
+import java.util.Properties;
+
+import org.jclouds.ContextBuilder;
+import org.jclouds.azure.storage.reference.AzureConstants;
+import org.jclouds.logging.config.NullLoggingModule;
+import org.jclouds.rest.internal.BaseRestApiTest;
+import org.testng.annotations.Test;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.inject.Module;
+
+@Test(groups = "unit")
+public class StorageUrlDelegateTest {
+
+ private static final String ACCOUNT = "foo";
+ private static final ImmutableSet<Module> MODULES = ImmutableSet.<Module> of(new BaseRestApiTest.MockModule(),
+ new NullLoggingModule());
+
+ @Test
+ void testDefaultEndpoint() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector().getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "https://foo.blob.core.windows.net/");
+ }
+
+ @Test
+ void testCustomEndpointWithoutStorageAccountPath() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob").endpoint("http://localhost:10000")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector()
+ .getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "http://localhost:10000/");
Review comment:
No worries, I appreciate the comments.
Sure, it defaults to "true". If the behaviour is explicitly configured to "false" in the scenario you've outlined, then you are right this would not be correct.
I don't think it's possible to prevent misconfigurations like this? I would not expect most users to override the default behaviour unless they had a reason to?
Did you have any suggestions?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] davidsloan commented on a change in pull request #88: JCLOUDS-1557 - Azure local server support
Posted by GitBox <gi...@apache.org>.
davidsloan commented on a change in pull request #88:
URL: https://github.com/apache/jclouds/pull/88#discussion_r522228624
##########
File path: providers/azureblob/src/test/java/org/jclouds/azure/storage/util/StorageUrlDelegateTest.java
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * 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.azure.storage.util;
+
+import static org.testng.Assert.assertEquals;
+
+import java.util.Properties;
+
+import org.jclouds.ContextBuilder;
+import org.jclouds.azure.storage.reference.AzureConstants;
+import org.jclouds.logging.config.NullLoggingModule;
+import org.jclouds.rest.internal.BaseRestApiTest;
+import org.testng.annotations.Test;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.inject.Module;
+
+@Test(groups = "unit")
+public class StorageUrlDelegateTest {
+
+ private static final String ACCOUNT = "foo";
+ private static final ImmutableSet<Module> MODULES = ImmutableSet.<Module> of(new BaseRestApiTest.MockModule(),
+ new NullLoggingModule());
+
+ @Test
+ void testDefaultEndpoint() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector().getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "https://foo.blob.core.windows.net/");
+ }
+
+ @Test
+ void testCustomEndpointWithoutStorageAccountPath() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob").endpoint("http://localhost:10000")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector()
+ .getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "http://localhost:10000/");
Review comment:
For azure, it's already present in the vhost/subdomain, having it in the path too would be incorrect. Therefore it must be configurable as the behaviour of the azurite endpoint looks for it in the path.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] davidsloan commented on a change in pull request #88: JCLOUDS-1557 - Azure local server support
Posted by GitBox <gi...@apache.org>.
davidsloan commented on a change in pull request #88:
URL: https://github.com/apache/jclouds/pull/88#discussion_r522306620
##########
File path: providers/azureblob/src/test/java/org/jclouds/azure/storage/util/StorageUrlDelegateTest.java
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * 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.azure.storage.util;
+
+import static org.testng.Assert.assertEquals;
+
+import java.util.Properties;
+
+import org.jclouds.ContextBuilder;
+import org.jclouds.azure.storage.reference.AzureConstants;
+import org.jclouds.logging.config.NullLoggingModule;
+import org.jclouds.rest.internal.BaseRestApiTest;
+import org.testng.annotations.Test;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.inject.Module;
+
+@Test(groups = "unit")
+public class StorageUrlDelegateTest {
+
+ private static final String ACCOUNT = "foo";
+ private static final ImmutableSet<Module> MODULES = ImmutableSet.<Module> of(new BaseRestApiTest.MockModule(),
+ new NullLoggingModule());
+
+ @Test
+ void testDefaultEndpoint() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector().getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "https://foo.blob.core.windows.net/");
+ }
+
+ @Test
+ void testCustomEndpointWithoutStorageAccountPath() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob").endpoint("http://localhost:10000")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector()
+ .getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "http://localhost:10000/");
Review comment:
Possibly. I was following the pattern used by the AWS S3 connector which has a similar property.
But I'm not sure there's any harm in having it configurable instead of automatic?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] gaul merged pull request #88: JCLOUDS-1557 - Azure local server support
Posted by GitBox <gi...@apache.org>.
gaul merged pull request #88:
URL: https://github.com/apache/jclouds/pull/88
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] nacx commented on a change in pull request #88: JCLOUDS-1557 - Azure local server support
Posted by GitBox <gi...@apache.org>.
nacx commented on a change in pull request #88:
URL: https://github.com/apache/jclouds/pull/88#discussion_r522859800
##########
File path: providers/azureblob/src/test/java/org/jclouds/azure/storage/util/StorageUrlDelegateTest.java
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * 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.azure.storage.util;
+
+import static org.testng.Assert.assertEquals;
+
+import java.util.Properties;
+
+import org.jclouds.ContextBuilder;
+import org.jclouds.azure.storage.reference.AzureConstants;
+import org.jclouds.logging.config.NullLoggingModule;
+import org.jclouds.rest.internal.BaseRestApiTest;
+import org.testng.annotations.Test;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.inject.Module;
+
+@Test(groups = "unit")
+public class StorageUrlDelegateTest {
+
+ private static final String ACCOUNT = "foo";
+ private static final ImmutableSet<Module> MODULES = ImmutableSet.<Module> of(new BaseRestApiTest.MockModule(),
+ new NullLoggingModule());
+
+ @Test
+ void testDefaultEndpoint() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector().getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "https://foo.blob.core.windows.net/");
+ }
+
+ @Test
+ void testCustomEndpointWithoutStorageAccountPath() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob").endpoint("http://localhost:10000")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector()
+ .getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "http://localhost:10000/");
Review comment:
The problem here is that given random endpoints, we can't preconfigure a correct way of setting the account for every possible case. What bout this:
Let's change the `StorageUrlDelegate` to an interface like this:
```java
@ImplementedBy(StorageAccountInVhost.class)
public interface StorageURLSupplier extends Supplier<URI> {
}
```
And have the default implementation be `StorageAccountInVhost`, which just keeps the current behavior, with the hardcoded endpoint and the identity in the host.
Then we could have a:
```java
public class AppendAccountToEndpoint implements StorageURLSupplier
```
That just appends the identity to whatever endpoint is configured.
To make it easier to configure, we could have a Guice module as well that does this (but is not included by default in the azure blob modules list:
```java
public class AppendAccountToEndpointModule extends AbstractModule {
@Override
protected void configure() {
bind(StorageURLSupplier.class).to(AppendAccountToEndpoint.class);
}
}
```
With these three pieces there, you can plug in your storage URL strategy when creating the context by adding the `AppendAccountToEndpointModule` to the list of modules.
This way we don't need the property, achieve the same behavior, but we also allow users to customize this further, as they can provide their own module to configure how the identity should be added to the endpoint.
This approach may seem overkill, but it ensures correctness in configuration.
WDYT?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] davidsloan commented on pull request #88: JCLOUDS-1557 - Azure local server support
Posted by GitBox <gi...@apache.org>.
davidsloan commented on pull request #88:
URL: https://github.com/apache/jclouds/pull/88#issuecomment-739998437
> Please make sure Azure integration tests pass:
>
> ```
> mvn integration-test -pl :azureblob -Plive -Dtest.azureblob.identity="${JCLOUDS_IDENTITY}" -Dtest.azureblob.credential="${JCLOUDS_CREDENTIAL}"
> ```
@gaul There seem to be 4 failures when I run locally, these are present on `master` as well if I run the command you supplied so I don't think they can be related to this change.
- testCopyIfNoneMatchNegative(org.jclouds.azureblob.blobstore.integration.AzureBlobIntegrationLiveTest)
- testPutMultipartInputStream(org.jclouds.azureblob.blobstore.integration.AzureBlobIntegrationLiveTest)
- testCopyBlobIfModifiedSince(org.jclouds.azureblob.AzureBlobClientLiveTest)
- testCopyBlobIfNoneMatch(org.jclouds.azureblob.AzureBlobClientLiveTest)
> @davidsloan We plan to release 2.3.0 later this month. Do you think you can finish this PR soon?
Hopefully there are no further changes to be made. :+1:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] davidsloan commented on a change in pull request #88: JCLOUDS-1557 - Azure local server support
Posted by GitBox <gi...@apache.org>.
davidsloan commented on a change in pull request #88:
URL: https://github.com/apache/jclouds/pull/88#discussion_r537592491
##########
File path: providers/azureblob/src/test/java/org/jclouds/azure/storage/util/StorageUrlDelegateTest.java
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * 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.azure.storage.util;
+
+import static org.testng.Assert.assertEquals;
+
+import java.util.Properties;
+
+import org.jclouds.ContextBuilder;
+import org.jclouds.azure.storage.reference.AzureConstants;
+import org.jclouds.logging.config.NullLoggingModule;
+import org.jclouds.rest.internal.BaseRestApiTest;
+import org.testng.annotations.Test;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.inject.Module;
+
+@Test(groups = "unit")
+public class StorageUrlDelegateTest {
+
+ private static final String ACCOUNT = "foo";
+ private static final ImmutableSet<Module> MODULES = ImmutableSet.<Module> of(new BaseRestApiTest.MockModule(),
+ new NullLoggingModule());
+
+ @Test
+ void testDefaultEndpoint() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector().getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "https://foo.blob.core.windows.net/");
+ }
+
+ @Test
+ void testCustomEndpointWithoutStorageAccountPath() {
+
+ StorageUrlDelegate target = ContextBuilder
+ .newBuilder("azureblob").endpoint("http://localhost:10000")
+ .credentials(ACCOUNT, "?token")
+ .modules(MODULES)
+ .buildInjector()
+ .getInstance(StorageUrlDelegate.class);
+
+ assertEquals(target.configureStorageUrl(), "http://localhost:10000/");
Review comment:
Sounds good, I've refactored the solution to follow your approach. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org