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 2020/11/25 10:07:49 UTC

[GitHub] [nifi-minifi-cpp] lordgamez opened a new pull request #942: MINIFICPP-1410 Add permissions property support for Putfile processor

lordgamez opened a new pull request #942:
URL: https://github.com/apache/nifi-minifi-cpp/pull/942


   There are multiple changes done in this PR:
   - Fixed Boost library usage, as previously even if libboost was present on the system it was not used. Now if  `EXCLUDE_BOOST` is not defined and libboost is present on the system, it will be used marked with the `USE_BOOST` definition.
   - Tests with Boost usage moved from gcc-4.8 environment to ubuntu 20.04 as gcc-4.8 seems to be incompatible with Ubuntu 16.04 distribution's Boost library, causing segfaults and throwing std::bad_alloc exceptions.
   - `Permissions` and `Directory Permissions` properties added to `Putfile` processor for non-Windows systems for changing the permissions of the newly created output directories and the output file.
   - Default permissions for created directories are now `777 & umask` instead of the previous `700` to be in sync with the boost implementation of `boost::filesystem::create_directory`.
   - Fixed permission problems using the new properties in docker tests.
   - Some additional filesystem functionalities are now supported by boost implementations.
   ---------------------------------------
   Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
   
   In order to streamline the review of the contribution we ask you
   to ensure the following steps have been taken:
   
   ### For all changes:
   - [ ] Is there a JIRA ticket associated with this PR? Is it referenced
        in the commit message?
   
   - [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
   
   - [ ] Has your PR been rebased against the latest commit within the target branch (typically main)?
   
   - [ ] Is your initial contribution a single, squashed commit?
   
   ### For code changes:
   - [ ] 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?
   - [ ] If applicable, have you updated the NOTICE file?
   
   ### 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 results 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #942: MINIFICPP-1410 Add permissions property support for Putfile processor

Posted by GitBox <gi...@apache.org>.
arpadboda commented on a change in pull request #942:
URL: https://github.com/apache/nifi-minifi-cpp/pull/942#discussion_r532619290



##########
File path: extensions/standard-processors/processors/PutFile.h
##########
@@ -110,6 +114,22 @@ class PutFile : public core::Processor {
                const std::string &destDir);
   std::shared_ptr<logging::Logger> logger_;
   static std::shared_ptr<utils::IdGenerator> id_generator_;
+
+#ifndef WIN32
+  class FilePermissions {
+    static const uint32_t MINIMUM_INVALID_PERMISSIONS_VALUE = 1 << 9;

Review comment:
       Nice solution :)




----------------------------------------------------------------
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] [nifi-minifi-cpp] lordgamez commented on a change in pull request #942: MINIFICPP-1410 Add permissions property support for Putfile processor

Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #942:
URL: https://github.com/apache/nifi-minifi-cpp/pull/942#discussion_r533552705



##########
File path: extensions/standard-processors/processors/PutFile.cpp
##########
@@ -54,6 +54,19 @@ core::Property PutFile::CreateDirs("Create Missing Directories", "If true, then
 core::Property PutFile::MaxDestFiles(
     core::PropertyBuilder::createProperty("Maximum File Count")->withDescription("Specifies the maximum number of files that can exist in the output directory")->withDefaultValue<int>(-1)->build());
 
+#ifndef WIN32
+core::Property PutFile::Permissions(
+    core::PropertyBuilder::createProperty("Permissions")
+      ->withDescription("Sets the permissions on the output file to the value of this attribute. "
+                        "Format must be format octal number (e.g. 644 or 0755). Not supported on Windows systems.")
+      ->build());
+core::Property PutFile::DirectoryPermissions(
+    core::PropertyBuilder::createProperty("Directory Permissions")
+      ->withDescription("Sets the permissions on the directories being created if 'Create Missing Directories' property is set. "
+                        "Format must be format octal number (e.g. 644 or 0755). Not supported on Windows systems.")
+      ->build());

Review comment:
       In that case it works the same way as `boost::filesystem::create_directory` which is creating the directory in mode `777` then applies the system's default umask.




----------------------------------------------------------------
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] [nifi-minifi-cpp] fgerlits commented on a change in pull request #942: MINIFICPP-1410 Add permissions property support for Putfile processor

Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #942:
URL: https://github.com/apache/nifi-minifi-cpp/pull/942#discussion_r533520945



##########
File path: PROCESSORS.md
##########
@@ -899,6 +899,8 @@ In the list below, the names of required properties appear in bold. Any other pr
 |**Create Missing Directories**|true||If true, then missing destination directories will be created. If false, flowfiles are penalized and sent to failure.|
 |Directory|.||The output directory to which to put files<br/>**Supports Expression Language: true**|
 |Maximum File Count|-1||Specifies the maximum number of files that can exist in the output directory|
+|Permissions|||Sets the permissions on the output file to the value of this attribute. Format must be in octal number (e.g. 644 or 0755). Not supported on Windows systems.|
+|Directory Permissions|||Sets the permissions on the directories being created if 'Create Missing Directories' property is set. Format must be format octal number (e.g. 644 or 0755). Not supported on Windows systems.|

Review comment:
       Typo: two "format"s.  I would get rid of all "format"s and write "Must be an octal number (e.g. 644 or 0755)." in both property descriptions.

##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -303,55 +322,74 @@ inline bool get_uid_gid(const std::string &path, uint64_t &uid, uint64_t &gid) {
 #endif
 
 inline int is_directory(const char * path) {
-    struct stat dir_stat;
-    if (stat(path, &dir_stat) < 0) {
-        return 0;
-    }
-    return S_ISDIR(dir_stat.st_mode);
+  struct stat dir_stat;
+  if (stat(path, &dir_stat) < 0) {
+      return 0;
+  }
+  return S_ISDIR(dir_stat.st_mode);
+}
+
+inline int exists(const std::string& path) {

Review comment:
       I can see that returning 0 (true) and -1 (false) fits into the existing pattern, but I think it would be better to return a `bool` at least from `is_directory()` and `exists()`.

##########
File path: extensions/standard-processors/processors/PutFile.cpp
##########
@@ -54,6 +54,19 @@ core::Property PutFile::CreateDirs("Create Missing Directories", "If true, then
 core::Property PutFile::MaxDestFiles(
     core::PropertyBuilder::createProperty("Maximum File Count")->withDescription("Specifies the maximum number of files that can exist in the output directory")->withDefaultValue<int>(-1)->build());
 
+#ifndef WIN32
+core::Property PutFile::Permissions(
+    core::PropertyBuilder::createProperty("Permissions")
+      ->withDescription("Sets the permissions on the output file to the value of this attribute. "
+                        "Format must be format octal number (e.g. 644 or 0755). Not supported on Windows systems.")

Review comment:
       even more "format"s here :)
   same suggestion as above

##########
File path: extensions/standard-processors/processors/PutFile.cpp
##########
@@ -54,6 +54,19 @@ core::Property PutFile::CreateDirs("Create Missing Directories", "If true, then
 core::Property PutFile::MaxDestFiles(
     core::PropertyBuilder::createProperty("Maximum File Count")->withDescription("Specifies the maximum number of files that can exist in the output directory")->withDefaultValue<int>(-1)->build());
 
+#ifndef WIN32
+core::Property PutFile::Permissions(
+    core::PropertyBuilder::createProperty("Permissions")
+      ->withDescription("Sets the permissions on the output file to the value of this attribute. "
+                        "Format must be format octal number (e.g. 644 or 0755). Not supported on Windows systems.")
+      ->build());
+core::Property PutFile::DirectoryPermissions(
+    core::PropertyBuilder::createProperty("Directory Permissions")
+      ->withDescription("Sets the permissions on the directories being created if 'Create Missing Directories' property is set. "
+                        "Format must be format octal number (e.g. 644 or 0755). Not supported on Windows systems.")
+      ->build());

Review comment:
       What happens if these are not set?  I would have expected a default value here.




----------------------------------------------------------------
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] [nifi-minifi-cpp] lordgamez commented on a change in pull request #942: MINIFICPP-1410 Add permissions property support for Putfile processor

Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #942:
URL: https://github.com/apache/nifi-minifi-cpp/pull/942#discussion_r533564990



##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -303,55 +322,74 @@ inline bool get_uid_gid(const std::string &path, uint64_t &uid, uint64_t &gid) {
 #endif
 
 inline int is_directory(const char * path) {
-    struct stat dir_stat;
-    if (stat(path, &dir_stat) < 0) {
-        return 0;
-    }
-    return S_ISDIR(dir_stat.st_mode);
+  struct stat dir_stat;
+  if (stat(path, &dir_stat) < 0) {
+      return 0;
+  }
+  return S_ISDIR(dir_stat.st_mode);
+}
+
+inline int exists(const std::string& path) {

Review comment:
       Fixed in [163bbea](https://github.com/apache/nifi-minifi-cpp/pull/942/commits/163bbeaa1f80756ff7d9b3f034e4e0bc71c3dcee)

##########
File path: extensions/standard-processors/processors/PutFile.cpp
##########
@@ -54,6 +54,19 @@ core::Property PutFile::CreateDirs("Create Missing Directories", "If true, then
 core::Property PutFile::MaxDestFiles(
     core::PropertyBuilder::createProperty("Maximum File Count")->withDescription("Specifies the maximum number of files that can exist in the output directory")->withDefaultValue<int>(-1)->build());
 
+#ifndef WIN32
+core::Property PutFile::Permissions(
+    core::PropertyBuilder::createProperty("Permissions")
+      ->withDescription("Sets the permissions on the output file to the value of this attribute. "
+                        "Format must be format octal number (e.g. 644 or 0755). Not supported on Windows systems.")

Review comment:
       Fixed in [163bbea](https://github.com/apache/nifi-minifi-cpp/pull/942/commits/163bbeaa1f80756ff7d9b3f034e4e0bc71c3dcee)

##########
File path: PROCESSORS.md
##########
@@ -899,6 +899,8 @@ In the list below, the names of required properties appear in bold. Any other pr
 |**Create Missing Directories**|true||If true, then missing destination directories will be created. If false, flowfiles are penalized and sent to failure.|
 |Directory|.||The output directory to which to put files<br/>**Supports Expression Language: true**|
 |Maximum File Count|-1||Specifies the maximum number of files that can exist in the output directory|
+|Permissions|||Sets the permissions on the output file to the value of this attribute. Format must be in octal number (e.g. 644 or 0755). Not supported on Windows systems.|
+|Directory Permissions|||Sets the permissions on the directories being created if 'Create Missing Directories' property is set. Format must be format octal number (e.g. 644 or 0755). Not supported on Windows systems.|

Review comment:
       Fixed in [163bbea](https://github.com/apache/nifi-minifi-cpp/pull/942/commits/163bbeaa1f80756ff7d9b3f034e4e0bc71c3dcee)




----------------------------------------------------------------
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] [nifi-minifi-cpp] arpadboda closed pull request #942: MINIFICPP-1410 Add permissions property support for Putfile processor

Posted by GitBox <gi...@apache.org>.
arpadboda closed pull request #942:
URL: https://github.com/apache/nifi-minifi-cpp/pull/942


   


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