Re: [PR] MINIFICPP-2614 Add more authentication options to Azure processors [nifi-minifi-cpp]

["szaszm (via GitHub)" <gi...@apache.org>]

szaszm commented on code in PR #2016:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2016#discussion_r2308193434


##########
extensions/azure/utils/AzureEnums.h:
##########
@@ -19,11 +19,39 @@
  */
 #pragma once
 
+#include "magic_enum.hpp"
+
 namespace org::apache::nifi::minifi::azure {
 
 enum class EntityTracking {
   none,
   timestamps
 };
 
+enum class CredentialConfigurationStrategyOption {
+  fromProperties,
+  defaultCredential,
+  managedIdentity,
+  workloadIdentity
+};

Review Comment:
   I'd prefer to keep the enum members as UpperCamelCase or UPPER_SNAKE_CASE



##########
extensions/azure/controllerservices/AzureStorageCredentialsService.h:
##########
@@ -42,33 +43,44 @@ class AzureStorageCredentialsService : public core::controller::ControllerServic
       .build();
   EXTENSIONAPI static constexpr auto StorageAccountKey = core::PropertyDefinitionBuilder<>::createProperty("Storage Account Key")
       .withDescription("The storage account key. This is an admin-like password providing access to every container in this account. "
-          "It is recommended one uses Shared Access Signature (SAS) token instead for fine-grained control with policies.")
+                       "It is recommended one uses Shared Access Signature (SAS) token instead for fine-grained control with policies "
+                       "if Credential Configuration Strategy is set to From Properties. If set, SAS Token must be empty.")

Review Comment:
   I'd prefer to keep continuation indentations = two normal levels of indentation, and not aligned with whatever from the previous lines.
   Why?
   - saves horizontal space
   - prevents refactor/rename from messing up the formatting
   - still lets us differentiate between normal indentation and continuation of the previous thing



##########
extensions/azure/controllerservices/AzureStorageCredentialsService.cpp:
##########
@@ -29,6 +29,10 @@ void AzureStorageCredentialsService::initialize() {
 }
 
 void AzureStorageCredentialsService::onEnable() {
+  if (auto credential_configuration_strategy = magic_enum::enum_cast<CredentialConfigurationStrategyOption>(
+        getProperty(CredentialConfigurationStrategy.name).value_or(std::string{magic_enum::enum_name(CredentialConfigurationStrategyOption::fromProperties)}))) {

Review Comment:
   Instead of breaking the line in two, I'd extract parts of it to keep it simpler



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