pvillard31 commented on code in PR #9635:
URL: https://github.com/apache/nifi/pull/9635#discussion_r1917214840
##########
nifi-extension-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/credentials/factory/CredentialPropertyDescriptors.java:
##########
@@ -91,4 +91,24 @@ private CredentialPropertyDescriptors() { }
.sensitive(true)
.build();
+ public static final PropertyDescriptor DELEGATION_STRATEGY = new
PropertyDescriptor.Builder()
+ .name("delegation-strategy")
+ .displayName("Delegation Strategy")
Review Comment:
We're trying to get away from `.displayName`, we should only use `.name` for
new properties. I think it'd be worth for us to mark `displayName` as
deprecated on the API side to make this more obvious.
##########
nifi-extension-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/credentials/factory/CredentialPropertyDescriptors.java:
##########
@@ -91,4 +91,24 @@ private CredentialPropertyDescriptors() { }
.sensitive(true)
.build();
+ public static final PropertyDescriptor DELEGATION_STRATEGY = new
PropertyDescriptor.Builder()
+ .name("delegation-strategy")
+ .displayName("Delegation Strategy")
+ .required(false)
Review Comment:
Since it's a property with allowable values, I believe the convention is to
have it as required = true.
##########
nifi-extension-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/credentials/factory/strategies/AbstractServiceAccountCredentialsStrategy.java:
##########
@@ -37,7 +39,13 @@ public AbstractServiceAccountCredentialsStrategy(String
name, PropertyDescriptor
@Override
public GoogleCredentials getGoogleCredentials(Map<PropertyDescriptor,
String> properties, HttpTransportFactory transportFactory) throws IOException {
+ final String delegationStrategy =
properties.get(CredentialPropertyDescriptors.DELEGATION_STRATEGY);
+ if (delegationStrategy != null &&
delegationStrategy.equals(DelegationStrategy.DELEGATED_ACCOUNT.getValue())) {
+ final String delegationUser =
properties.get(CredentialPropertyDescriptors.DELEGATION_USER);
+ return
GoogleCredentials.fromStream(getServiceAccountJson(properties),
transportFactory).createDelegated(delegationUser);
+ } else {
return
GoogleCredentials.fromStream(getServiceAccountJson(properties),
transportFactory);
+ }
Review Comment:
Given this is an enum and it'll never be null, can't we use the switch case
return style here?
##########
nifi-extension-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/credentials/factory/CredentialPropertyDescriptors.java:
##########
@@ -91,4 +91,24 @@ private CredentialPropertyDescriptors() { }
.sensitive(true)
.build();
+ public static final PropertyDescriptor DELEGATION_STRATEGY = new
PropertyDescriptor.Builder()
+ .name("delegation-strategy")
+ .displayName("Delegation Strategy")
+ .required(false)
+ .defaultValue(DelegationStrategy.SERVICE_ACCOUNT)
+ .allowableValues(DelegationStrategy.class)
+ .description("The Delegation Strategy determines which account is
used when calls are made with the GCP Credential.")
+ .build();
+
+ public static final PropertyDescriptor DELEGATION_USER = new
PropertyDescriptor.Builder()
+ .name("delegation-user")
+ .displayName("Delegation User")
Review Comment:
Remove use of `displayName`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
