You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/07/19 13:48:05 UTC
[GitHub] [nifi] adenes opened a new pull request #5229: NIFI-8762 ADLSCredentialControllerService does not support EL for Storage Account name
adenes opened a new pull request #5229:
URL: https://github.com/apache/nifi/pull/5229
- Added FLOWFILE_ATTRIBUTES expression language support to the Storage Account Name and
and also to the Storage Account Key property to be consistent with
AzureStorageCredentialsControllerService
- ADLSCredentialControllerService.ACCOUNT_KEY and ADLSCredentialControllerService.SAS_TOKEN
PropertyDescriptor public constants are the same as AzureStorageUtils.ACCOUNT_KEY and
AzureStorageUtils.PROP_SAS_TOKEN respectively, but they haven't been removed to keep
backward compatibility.
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [x] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [x] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [x] Have you written or updated unit tests to verify your changes?
- [x] Have you verified that the full build is successful on JDK 8?
- [ ] Have you verified that the full build is successful on JDK 11?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] adenes commented on pull request #5229: NIFI-8762 ADLSCredentialControllerService does not support EL for Storage Account name
Posted by GitBox <gi...@apache.org>.
adenes commented on pull request #5229:
URL: https://github.com/apache/nifi/pull/5229#issuecomment-882667163
Thanks @exceptionfactory for the review, I have updated the PR according to your suggestion. I also added a test case to cover the changes.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5229: NIFI-8762 ADLSCredentialControllerService does not support EL for Storage Account name
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5229:
URL: https://github.com/apache/nifi/pull/5229#discussion_r672378707
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -65,16 +64,9 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
- public static final PropertyDescriptor ACCOUNT_KEY = new PropertyDescriptor.Builder()
- .fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_KEY)
- .description(AzureStorageUtils.ACCOUNT_KEY_BASE_DESCRIPTION)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
- .build();
+ public static final PropertyDescriptor ACCOUNT_KEY = AzureStorageUtils.ACCOUNT_KEY;
Review comment:
Instead of keeping the local `ACCOUNT_KEY` property descriptor, it would be better to replace existing references to use `AzureStorageUtils.ACCOUNT_KEY` directly.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -65,16 +64,9 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
- public static final PropertyDescriptor ACCOUNT_KEY = new PropertyDescriptor.Builder()
- .fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_KEY)
- .description(AzureStorageUtils.ACCOUNT_KEY_BASE_DESCRIPTION)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
- .build();
+ public static final PropertyDescriptor ACCOUNT_KEY = AzureStorageUtils.ACCOUNT_KEY;
- public static final PropertyDescriptor SAS_TOKEN = new PropertyDescriptor.Builder()
- .fromPropertyDescriptor(AzureStorageUtils.PROP_SAS_TOKEN)
- .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
- .build();
+ public static final PropertyDescriptor SAS_TOKEN = AzureStorageUtils.PROP_SAS_TOKEN;
Review comment:
Same as `ACCOUNT_KEY`, it would be better to replace references to `SAS_TOKEN` with `AzureStorageUtils.PROP_SAS_TOKEN`.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -65,16 +64,9 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
- public static final PropertyDescriptor ACCOUNT_KEY = new PropertyDescriptor.Builder()
- .fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_KEY)
- .description(AzureStorageUtils.ACCOUNT_KEY_BASE_DESCRIPTION)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
- .build();
+ public static final PropertyDescriptor ACCOUNT_KEY = AzureStorageUtils.ACCOUNT_KEY;
Review comment:
Thanks for following up. Although it is a public field, processor variables are not part of the public API contract, just the property names themselves. Since the change would still preserve the existing property name, refactoring seems like the best approach.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5229: NIFI-8762 ADLSCredentialControllerService does not support EL for Storage Account name
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5229:
URL: https://github.com/apache/nifi/pull/5229#discussion_r672378707
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -65,16 +64,9 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
- public static final PropertyDescriptor ACCOUNT_KEY = new PropertyDescriptor.Builder()
- .fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_KEY)
- .description(AzureStorageUtils.ACCOUNT_KEY_BASE_DESCRIPTION)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
- .build();
+ public static final PropertyDescriptor ACCOUNT_KEY = AzureStorageUtils.ACCOUNT_KEY;
Review comment:
Instead of keeping the local `ACCOUNT_KEY` property descriptor, it would be better to replace existing references to use `AzureStorageUtils.ACCOUNT_KEY` directly.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -65,16 +64,9 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
- public static final PropertyDescriptor ACCOUNT_KEY = new PropertyDescriptor.Builder()
- .fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_KEY)
- .description(AzureStorageUtils.ACCOUNT_KEY_BASE_DESCRIPTION)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
- .build();
+ public static final PropertyDescriptor ACCOUNT_KEY = AzureStorageUtils.ACCOUNT_KEY;
- public static final PropertyDescriptor SAS_TOKEN = new PropertyDescriptor.Builder()
- .fromPropertyDescriptor(AzureStorageUtils.PROP_SAS_TOKEN)
- .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
- .build();
+ public static final PropertyDescriptor SAS_TOKEN = AzureStorageUtils.PROP_SAS_TOKEN;
Review comment:
Same as `ACCOUNT_KEY`, it would be better to replace references to `SAS_TOKEN` with `AzureStorageUtils.PROP_SAS_TOKEN`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] adenes commented on a change in pull request #5229: NIFI-8762 ADLSCredentialControllerService does not support EL for Storage Account name
Posted by GitBox <gi...@apache.org>.
adenes commented on a change in pull request #5229:
URL: https://github.com/apache/nifi/pull/5229#discussion_r672392102
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -65,16 +64,9 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
- public static final PropertyDescriptor ACCOUNT_KEY = new PropertyDescriptor.Builder()
- .fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_KEY)
- .description(AzureStorageUtils.ACCOUNT_KEY_BASE_DESCRIPTION)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
- .build();
+ public static final PropertyDescriptor ACCOUNT_KEY = AzureStorageUtils.ACCOUNT_KEY;
Review comment:
I was hesitant to remove this (actually first I removed then decided to keep it this way) because this is a public field so theoretically this could have been used by other (3rd party) processors so removing it would be a breaking change.
But I don't have a strong opinion on this. Would you remove it, @exceptionfactory ?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] adenes commented on a change in pull request #5229: NIFI-8762 ADLSCredentialControllerService does not support EL for Storage Account name
Posted by GitBox <gi...@apache.org>.
adenes commented on a change in pull request #5229:
URL: https://github.com/apache/nifi/pull/5229#discussion_r672323268
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -50,9 +50,8 @@
public static final PropertyDescriptor ACCOUNT_NAME = new PropertyDescriptor.Builder()
.fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_NAME)
- .description(AzureStorageUtils.ACCOUNT_NAME_BASE_DESCRIPTION)
+ .description(AzureStorageUtils.ACCOUNT_NAME_BASE_DESCRIPTION + AzureStorageUtils.ACCOUNT_NAME_SECURITY_DESCRIPTION)
.required(true)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
Review comment:
The builder is instantiated with `fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_NAME)` so it inherits `AzureStorageUtils.ACCOUNT_NAME`'s properties, including the expression language scope.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] adenes commented on pull request #5229: NIFI-8762 ADLSCredentialControllerService does not support EL for Storage Account name
Posted by GitBox <gi...@apache.org>.
adenes commented on pull request #5229:
URL: https://github.com/apache/nifi/pull/5229#issuecomment-882667163
Thanks @exceptionfactory for the review, I have updated the PR according to your suggestion. I also added a test case to cover the changes.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] adenes commented on a change in pull request #5229: NIFI-8762 ADLSCredentialControllerService does not support EL for Storage Account name
Posted by GitBox <gi...@apache.org>.
adenes commented on a change in pull request #5229:
URL: https://github.com/apache/nifi/pull/5229#discussion_r672408192
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -65,16 +64,9 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
- public static final PropertyDescriptor ACCOUNT_KEY = new PropertyDescriptor.Builder()
- .fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_KEY)
- .description(AzureStorageUtils.ACCOUNT_KEY_BASE_DESCRIPTION)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
- .build();
+ public static final PropertyDescriptor ACCOUNT_KEY = AzureStorageUtils.ACCOUNT_KEY;
Review comment:
gotcha, updating the PR, 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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] pvillard31 commented on a change in pull request #5229: NIFI-8762 ADLSCredentialControllerService does not support EL for Storage Account name
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on a change in pull request #5229:
URL: https://github.com/apache/nifi/pull/5229#discussion_r672320236
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -50,9 +50,8 @@
public static final PropertyDescriptor ACCOUNT_NAME = new PropertyDescriptor.Builder()
.fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_NAME)
- .description(AzureStorageUtils.ACCOUNT_NAME_BASE_DESCRIPTION)
+ .description(AzureStorageUtils.ACCOUNT_NAME_BASE_DESCRIPTION + AzureStorageUtils.ACCOUNT_NAME_SECURITY_DESCRIPTION)
.required(true)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
Review comment:
Are we sure this is enough to remove this line? I think the default is `NONE` when not specified.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -50,9 +50,8 @@
public static final PropertyDescriptor ACCOUNT_NAME = new PropertyDescriptor.Builder()
.fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_NAME)
- .description(AzureStorageUtils.ACCOUNT_NAME_BASE_DESCRIPTION)
+ .description(AzureStorageUtils.ACCOUNT_NAME_BASE_DESCRIPTION + AzureStorageUtils.ACCOUNT_NAME_SECURITY_DESCRIPTION)
.required(true)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
Review comment:
Oh good point, thanks @adenes !
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] adenes commented on pull request #5229: NIFI-8762 ADLSCredentialControllerService does not support EL for Storage Account name
Posted by GitBox <gi...@apache.org>.
adenes commented on pull request #5229:
URL: https://github.com/apache/nifi/pull/5229#issuecomment-882667163
Thanks @exceptionfactory for the review, I have updated the PR according to your suggestion. I also added a test case to cover the changes.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] pvillard31 commented on a change in pull request #5229: NIFI-8762 ADLSCredentialControllerService does not support EL for Storage Account name
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on a change in pull request #5229:
URL: https://github.com/apache/nifi/pull/5229#discussion_r672320236
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -50,9 +50,8 @@
public static final PropertyDescriptor ACCOUNT_NAME = new PropertyDescriptor.Builder()
.fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_NAME)
- .description(AzureStorageUtils.ACCOUNT_NAME_BASE_DESCRIPTION)
+ .description(AzureStorageUtils.ACCOUNT_NAME_BASE_DESCRIPTION + AzureStorageUtils.ACCOUNT_NAME_SECURITY_DESCRIPTION)
.required(true)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
Review comment:
Are we sure this is enough to remove this line? I think the default is `NONE` when not specified.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] adenes commented on a change in pull request #5229: NIFI-8762 ADLSCredentialControllerService does not support EL for Storage Account name
Posted by GitBox <gi...@apache.org>.
adenes commented on a change in pull request #5229:
URL: https://github.com/apache/nifi/pull/5229#discussion_r672323268
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -50,9 +50,8 @@
public static final PropertyDescriptor ACCOUNT_NAME = new PropertyDescriptor.Builder()
.fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_NAME)
- .description(AzureStorageUtils.ACCOUNT_NAME_BASE_DESCRIPTION)
+ .description(AzureStorageUtils.ACCOUNT_NAME_BASE_DESCRIPTION + AzureStorageUtils.ACCOUNT_NAME_SECURITY_DESCRIPTION)
.required(true)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
Review comment:
The builder is instantiated with `fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_NAME)` so it inherits `AzureStorageUtils.ACCOUNT_NAME`'s properties, including the expression language scope.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -65,16 +64,9 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
- public static final PropertyDescriptor ACCOUNT_KEY = new PropertyDescriptor.Builder()
- .fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_KEY)
- .description(AzureStorageUtils.ACCOUNT_KEY_BASE_DESCRIPTION)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
- .build();
+ public static final PropertyDescriptor ACCOUNT_KEY = AzureStorageUtils.ACCOUNT_KEY;
Review comment:
I was hesitant to remove this (actually first I removed then decided to keep it this way) because this is a public field so theoretically this could have been used by other (3rd party) processors so removing it would be a breaking change.
But I don't have a strong opinion on this. Would you remove it, @exceptionfactory ?
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -65,16 +64,9 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
- public static final PropertyDescriptor ACCOUNT_KEY = new PropertyDescriptor.Builder()
- .fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_KEY)
- .description(AzureStorageUtils.ACCOUNT_KEY_BASE_DESCRIPTION)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
- .build();
+ public static final PropertyDescriptor ACCOUNT_KEY = AzureStorageUtils.ACCOUNT_KEY;
Review comment:
gotcha, updating the PR, 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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] pvillard31 commented on a change in pull request #5229: NIFI-8762 ADLSCredentialControllerService does not support EL for Storage Account name
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on a change in pull request #5229:
URL: https://github.com/apache/nifi/pull/5229#discussion_r672320236
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -50,9 +50,8 @@
public static final PropertyDescriptor ACCOUNT_NAME = new PropertyDescriptor.Builder()
.fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_NAME)
- .description(AzureStorageUtils.ACCOUNT_NAME_BASE_DESCRIPTION)
+ .description(AzureStorageUtils.ACCOUNT_NAME_BASE_DESCRIPTION + AzureStorageUtils.ACCOUNT_NAME_SECURITY_DESCRIPTION)
.required(true)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
Review comment:
Are we sure this is enough to remove this line? I think the default is `NONE` when not specified.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -50,9 +50,8 @@
public static final PropertyDescriptor ACCOUNT_NAME = new PropertyDescriptor.Builder()
.fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_NAME)
- .description(AzureStorageUtils.ACCOUNT_NAME_BASE_DESCRIPTION)
+ .description(AzureStorageUtils.ACCOUNT_NAME_BASE_DESCRIPTION + AzureStorageUtils.ACCOUNT_NAME_SECURITY_DESCRIPTION)
.required(true)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
Review comment:
Oh good point, thanks @adenes !
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5229: NIFI-8762 ADLSCredentialControllerService does not support EL for Storage Account name
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5229:
URL: https://github.com/apache/nifi/pull/5229#discussion_r672394977
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -65,16 +64,9 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
- public static final PropertyDescriptor ACCOUNT_KEY = new PropertyDescriptor.Builder()
- .fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_KEY)
- .description(AzureStorageUtils.ACCOUNT_KEY_BASE_DESCRIPTION)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
- .build();
+ public static final PropertyDescriptor ACCOUNT_KEY = AzureStorageUtils.ACCOUNT_KEY;
Review comment:
Thanks for following up. Although it is a public field, processor variables are not part of the public API contract, just the property names themselves. Since the change would still preserve the existing property name, refactoring seems like the best approach.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] pvillard31 commented on a change in pull request #5229: NIFI-8762 ADLSCredentialControllerService does not support EL for Storage Account name
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on a change in pull request #5229:
URL: https://github.com/apache/nifi/pull/5229#discussion_r672333324
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -50,9 +50,8 @@
public static final PropertyDescriptor ACCOUNT_NAME = new PropertyDescriptor.Builder()
.fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_NAME)
- .description(AzureStorageUtils.ACCOUNT_NAME_BASE_DESCRIPTION)
+ .description(AzureStorageUtils.ACCOUNT_NAME_BASE_DESCRIPTION + AzureStorageUtils.ACCOUNT_NAME_SECURITY_DESCRIPTION)
.required(true)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
Review comment:
Oh good point, thanks @adenes !
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] asfgit closed pull request #5229: NIFI-8762 ADLSCredentialControllerService does not support EL for Storage Account name
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #5229:
URL: https://github.com/apache/nifi/pull/5229
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5229: NIFI-8762 ADLSCredentialControllerService does not support EL for Storage Account name
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5229:
URL: https://github.com/apache/nifi/pull/5229#discussion_r672378707
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -65,16 +64,9 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
- public static final PropertyDescriptor ACCOUNT_KEY = new PropertyDescriptor.Builder()
- .fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_KEY)
- .description(AzureStorageUtils.ACCOUNT_KEY_BASE_DESCRIPTION)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
- .build();
+ public static final PropertyDescriptor ACCOUNT_KEY = AzureStorageUtils.ACCOUNT_KEY;
Review comment:
Instead of keeping the local `ACCOUNT_KEY` property descriptor, it would be better to replace existing references to use `AzureStorageUtils.ACCOUNT_KEY` directly.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -65,16 +64,9 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
- public static final PropertyDescriptor ACCOUNT_KEY = new PropertyDescriptor.Builder()
- .fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_KEY)
- .description(AzureStorageUtils.ACCOUNT_KEY_BASE_DESCRIPTION)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
- .build();
+ public static final PropertyDescriptor ACCOUNT_KEY = AzureStorageUtils.ACCOUNT_KEY;
- public static final PropertyDescriptor SAS_TOKEN = new PropertyDescriptor.Builder()
- .fromPropertyDescriptor(AzureStorageUtils.PROP_SAS_TOKEN)
- .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
- .build();
+ public static final PropertyDescriptor SAS_TOKEN = AzureStorageUtils.PROP_SAS_TOKEN;
Review comment:
Same as `ACCOUNT_KEY`, it would be better to replace references to `SAS_TOKEN` with `AzureStorageUtils.PROP_SAS_TOKEN`.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -65,16 +64,9 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
- public static final PropertyDescriptor ACCOUNT_KEY = new PropertyDescriptor.Builder()
- .fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_KEY)
- .description(AzureStorageUtils.ACCOUNT_KEY_BASE_DESCRIPTION)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
- .build();
+ public static final PropertyDescriptor ACCOUNT_KEY = AzureStorageUtils.ACCOUNT_KEY;
Review comment:
Thanks for following up. Although it is a public field, processor variables are not part of the public API contract, just the property names themselves. Since the change would still preserve the existing property name, refactoring seems like the best approach.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] adenes commented on a change in pull request #5229: NIFI-8762 ADLSCredentialControllerService does not support EL for Storage Account name
Posted by GitBox <gi...@apache.org>.
adenes commented on a change in pull request #5229:
URL: https://github.com/apache/nifi/pull/5229#discussion_r672323268
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -50,9 +50,8 @@
public static final PropertyDescriptor ACCOUNT_NAME = new PropertyDescriptor.Builder()
.fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_NAME)
- .description(AzureStorageUtils.ACCOUNT_NAME_BASE_DESCRIPTION)
+ .description(AzureStorageUtils.ACCOUNT_NAME_BASE_DESCRIPTION + AzureStorageUtils.ACCOUNT_NAME_SECURITY_DESCRIPTION)
.required(true)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
Review comment:
The builder is instantiated with `fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_NAME)` so it inherits `AzureStorageUtils.ACCOUNT_NAME`'s properties, including the expression language scope.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -65,16 +64,9 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
- public static final PropertyDescriptor ACCOUNT_KEY = new PropertyDescriptor.Builder()
- .fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_KEY)
- .description(AzureStorageUtils.ACCOUNT_KEY_BASE_DESCRIPTION)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
- .build();
+ public static final PropertyDescriptor ACCOUNT_KEY = AzureStorageUtils.ACCOUNT_KEY;
Review comment:
I was hesitant to remove this (actually first I removed then decided to keep it this way) because this is a public field so theoretically this could have been used by other (3rd party) processors so removing it would be a breaking change.
But I don't have a strong opinion on this. Would you remove it, @exceptionfactory ?
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -65,16 +64,9 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
- public static final PropertyDescriptor ACCOUNT_KEY = new PropertyDescriptor.Builder()
- .fromPropertyDescriptor(AzureStorageUtils.ACCOUNT_KEY)
- .description(AzureStorageUtils.ACCOUNT_KEY_BASE_DESCRIPTION)
- .expressionLanguageSupported(ExpressionLanguageScope.NONE)
- .build();
+ public static final PropertyDescriptor ACCOUNT_KEY = AzureStorageUtils.ACCOUNT_KEY;
Review comment:
gotcha, updating the PR, 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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org