nifi-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [nifi] sjyang18 commented on a change in pull request #4265: NIFI-7434: Endpoint suffix property in AzureStorageAccount NIFI processors
Date Wed, 27 May 2020 03:52:18 GMT

sjyang18 commented on a change in pull request #4265:
URL: https://github.com/apache/nifi/pull/4265#discussion_r430548677



##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/test/java/org/apache/nifi/services/azure/storage/TestAzureStorageCredentialsControllerServiceLookup.java
##########
@@ -71,28 +73,32 @@ public void testLookupServiceA() {
         final AzureStorageCredentialsDetails storageCredentialsDetails = lookupService.getStorageCredentialsDetails(attributes);
         assertNotNull(storageCredentialsDetails);
         assertEquals("Account_A", storageCredentialsDetails.getStorageAccountName());
+        assertEquals("accountsuffix.core.windows.net", storageCredentialsDetails.getStorageSuffix());
     }
 
     @Test
     public void testLookupServiceB() {

Review comment:
       null default is handled in SDK. So, the current behavior does not get affected.
   
   
   private static String getDNS(String service, String base) {
           if (base == null) {
               base = DEFAULT_DNS;
           }
           return String.format(DNS_NAME_FORMAT, service, base);
    }
   

##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/utils/AzureStorageUtils.java
##########
@@ -85,6 +86,20 @@
             .sensitive(true)
             .build();
 
+    public static final PropertyDescriptor ENDPOINT_SUFFIX = new PropertyDescriptor.Builder()
+            .name("storage-endpoint-suffix")
+            .displayName("Common Storage Account Endpoint Suffix")
+            .description(
+                    "Storage accounts in public Azure always use a common FQDN suffix. "
+
+                    "Override this endpoint suffix with a different suffix in certain circumsances
(like Azure Stack or non-public Azure regions). " +
+                    "The preferred way is to configure them through a controller service
specified in the Storage Credentials property. " +
+                    "The controller service can provide a common/shared configuration for
multiple/all Azure processors. Furthermore, the credentials " +
+                    "can also be looked up dynamically with the 'Lookup' version of the service.")
+            .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+            .required(false)

Review comment:
       Null default value is acceptable. The Azure Storage SDK sets the default end suffix,
if we pass null. Do we still need a validator in this case? I validated the regression behavior
without setting end suffix property.

##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/utils/AzureStorageUtils.java
##########
@@ -85,6 +86,20 @@
             .sensitive(true)
             .build();
 
+    public static final PropertyDescriptor ENDPOINT_SUFFIX = new PropertyDescriptor.Builder()
+            .name("storage-endpoint-suffix")
+            .displayName("Common Storage Account Endpoint Suffix")
+            .description(
+                    "Storage accounts in public Azure always use a common FQDN suffix. "
+
+                    "Override this endpoint suffix with a different suffix in certain circumsances
(like Azure Stack or non-public Azure regions). " +
+                    "The preferred way is to configure them through a controller service
specified in the Storage Credentials property. " +
+                    "The controller service can provide a common/shared configuration for
multiple/all Azure processors. Furthermore, the credentials " +
+                    "can also be looked up dynamically with the 'Lookup' version of the service.")
+            .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+            .required(false)

Review comment:
       In order to support Azure  Stack on-premise, this should be an editable free form.


##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/utils/AzureStorageUtils.java
##########
@@ -85,6 +86,20 @@
             .sensitive(true)
             .build();
 
+    public static final PropertyDescriptor ENDPOINT_SUFFIX = new PropertyDescriptor.Builder()
+            .name("storage-endpoint-suffix")
+            .displayName("Common Storage Account Endpoint Suffix")
+            .description(
+                    "Storage accounts in public Azure always use a common FQDN suffix. "
+
+                    "Override this endpoint suffix with a different suffix in certain circumsances
(like Azure Stack or non-public Azure regions). " +
+                    "The preferred way is to configure them through a controller service
specified in the Storage Credentials property. " +
+                    "The controller service can provide a common/shared configuration for
multiple/all Azure processors. Furthermore, the credentials " +
+                    "can also be looked up dynamically with the 'Lookup' version of the service.")
+            .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)

Review comment:
       I took the option and set StandardValidators.NON_BLANK_VALIDATOR to EndpointSuffix
property. 

##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-services-api/src/main/java/org/apache/nifi/services/azure/storage/AzureStorageCredentialsDetails.java
##########
@@ -22,17 +22,24 @@
 
     private final String storageAccountName;
 
+    private final String storageSuffix;
+
     private final StorageCredentials storageCredentials;
 
-    public AzureStorageCredentialsDetails(String storageAccountName, StorageCredentials storageCredentials)
{
+    public AzureStorageCredentialsDetails(String storageAccountName, String storageSuffix,
StorageCredentials storageCredentials) {

Review comment:
       done. Thanks for the tip.

##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/test/java/org/apache/nifi/processors/azure/storage/queue/GetAzureQueueStorageIT.java
##########
@@ -39,6 +39,7 @@ public void setUp() throws StorageException {
         cloudQueue.addMessage(new CloudQueueMessage("Dummy Message 1"), 604800, 0, null,
null);
         cloudQueue.addMessage(new CloudQueueMessage("Dummy Message 2"), 604800, 0, null,
null);
         cloudQueue.addMessage(new CloudQueueMessage("Dummy Message 3"), 604800, 0, null,
null);
+

Review comment:
       Done. But, I am still updating this file with a test case.

##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/utils/AzureStorageUtils.java
##########
@@ -85,6 +86,20 @@
             .sensitive(true)
             .build();
 
+    public static final PropertyDescriptor ENDPOINT_SUFFIX = new PropertyDescriptor.Builder()
+            .name("storage-endpoint-suffix")
+            .displayName("Common Storage Account Endpoint Suffix")
+            .description(
+                    "Storage accounts in public Azure always use a common FQDN suffix. "
+
+                    "Override this endpoint suffix with a different suffix in certain circumsances
(like Azure Stack or non-public Azure regions). " +
+                    "The preferred way is to configure them through a controller service
specified in the Storage Credentials property. " +
+                    "The controller service can provide a common/shared configuration for
multiple/all Azure processors. Furthermore, the credentials " +
+                    "can also be looked up dynamically with the 'Lookup' version of the service.")
+            .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+            .required(false)

Review comment:
       I added NON_BLANK_VALIDATOR.




----------------------------------------------------------------
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



Mime
View raw message