You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "lordgamez (via GitHub)" <gi...@apache.org> on 2023/08/23 15:29:20 UTC
[GitHub] [nifi-minifi-cpp] lordgamez opened a new pull request, #1638: MINIFICPP-1774 Set properties from command line arguments
lordgamez opened a new pull request, #1638:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638
- Remove ArgParser class and use thirdparty argparse library instead
- Add -p/--property option to override configuration properties
https://issues.apache.org/jira/browse/MINIFICPP-1774
------------
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1638: MINIFICPP-1774 Set properties from command line arguments
Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1638:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638#discussion_r1307351648
##########
LICENSE:
##########
@@ -3416,3 +3416,13 @@ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.
+
+This product bundles 'argparse' which is available under The MIT License.
Review Comment:
we are already using `cxxopts` library for the minificontroller, does `argparse` provide some extra features?
--
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-minifi-cpp] szaszm commented on a diff in pull request #1638: MINIFICPP-1774 Set properties from command line arguments
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1638:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638#discussion_r1310217212
##########
minifi_main/MiNiFiMain.cpp:
##########
@@ -134,7 +135,93 @@ void writeJsonSchema(const std::shared_ptr<minifi::Configure> &configuration, st
out << minifi::docs::generateJsonSchema();
}
+void overridePropertiesFromCommandLine(const argparse::ArgumentParser& parser, const std::shared_ptr<minifi::Configure>& configure) {
+ const auto& properties = parser.get<std::vector<std::string>>("--property");
+ for (const auto& property : properties) {
+ auto property_key_and_value = utils::StringUtils::splitAndTrimRemovingEmpty(property, "=");
+ if (property_key_and_value.size() != 2) {
+ std::cerr << "Command line property must be defined in <key>=<value> format, invalid property: " << property << std::endl;
+ std::cerr << parser;
+ std::exit(1);
+ }
+ configure->set(property_key_and_value[0], property_key_and_value[1]);
+ }
+}
+
+void dumpDocsIfRequested(const argparse::ArgumentParser& parser, const std::shared_ptr<minifi::Configure>& configure) {
+ if (!parser.is_used("--docs")) {
+ return;
+ }
+ const auto& docs_params = parser.get<std::vector<std::string>>("--docs");
+ if (utils::file::create_dir(docs_params[0]) != 0) {
+ std::cerr << "Working directory doesn't exist and cannot be created: " << docs_params[0] << std::endl;
+ exit(1);
Review Comment:
Please keep the `exit` vs `std::exit` calls consistent in the same file
##########
minifi_main/MiNiFiMain.cpp:
##########
@@ -134,7 +135,93 @@ void writeJsonSchema(const std::shared_ptr<minifi::Configure> &configuration, st
out << minifi::docs::generateJsonSchema();
}
+void overridePropertiesFromCommandLine(const argparse::ArgumentParser& parser, const std::shared_ptr<minifi::Configure>& configure) {
+ const auto& properties = parser.get<std::vector<std::string>>("--property");
+ for (const auto& property : properties) {
+ auto property_key_and_value = utils::StringUtils::splitAndTrimRemovingEmpty(property, "=");
+ if (property_key_and_value.size() != 2) {
+ std::cerr << "Command line property must be defined in <key>=<value> format, invalid property: " << property << std::endl;
+ std::cerr << parser;
+ std::exit(1);
+ }
+ configure->set(property_key_and_value[0], property_key_and_value[1]);
+ }
+}
+
+void dumpDocsIfRequested(const argparse::ArgumentParser& parser, const std::shared_ptr<minifi::Configure>& configure) {
+ if (!parser.is_used("--docs")) {
+ return;
+ }
+ const auto& docs_params = parser.get<std::vector<std::string>>("--docs");
+ if (utils::file::create_dir(docs_params[0]) != 0) {
+ std::cerr << "Working directory doesn't exist and cannot be created: " << docs_params[0] << std::endl;
+ exit(1);
+ }
+
+ std::cout << "Dumping docs to " << docs_params[0] << std::endl;
+ if (docs_params.size() > 1) {
+ auto path = std::filesystem::path(docs_params[1]);
+ if (std::filesystem::exists(path) && !std::filesystem::is_regular_file(path)) {
+ std::cerr << "PROCESSORS.md target path exists, but it is not a regular file: " << path << std::endl;
+ exit(1);
+ }
+ auto dir = path.parent_path();
+ if (dir == docs_params[0]) {
+ std::cerr << "Target file should be out of the working directory: " << dir << std::endl;
+ exit(1);
+ }
+ std::ofstream outref(docs_params[1]);
+ dumpDocs(configure, docs_params[0], outref);
+ } else {
+ dumpDocs(configure, docs_params[0], std::cout);
+ }
+ exit(0);
+}
+
+void writeSchemaIfRequested(const argparse::ArgumentParser& parser, const std::shared_ptr<minifi::Configure>& configure) {
+ if (!parser.is_used("--schema")) {
+ return;
+ }
+ const auto& schema_path = parser.get("--schema");
+ if (std::filesystem::exists(schema_path) && !std::filesystem::is_regular_file(schema_path)) {
+ std::cerr << "JSON schema target path already exists, but it is not a regular file: " << schema_path << std::endl;
+ exit(1);
+ }
+
+ auto parent_dir = std::filesystem::path(schema_path).parent_path();
+ if (utils::file::create_dir(parent_dir) != 0) {
+ std::cerr << "JSON schema parent directory doesn't exist and cannot be created: " << parent_dir << std::endl;
+ exit(1);
+ }
+ std::cout << "Writing json schema to " << schema_path << std::endl;
+ std::ofstream schema_file{schema_path};
+ writeJsonSchema(configure, schema_file);
+ std::exit(0);
Review Comment:
`exit` doesn't unwind the stack, so local variable destructors are not called. I'd prefer either normal return from main, or at least closing all files and other resources before calling exit.
```suggestion
{
std::ofstream schema_file{schema_path};
writeJsonSchema(configure, schema_file);
}
std::exit(0);
```
--
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-minifi-cpp] lordgamez commented on a diff in pull request #1638: MINIFICPP-1774 Set properties from command line arguments
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1638:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638#discussion_r1311274597
##########
minifi_main/MiNiFiMain.cpp:
##########
@@ -134,7 +135,93 @@ void writeJsonSchema(const std::shared_ptr<minifi::Configure> &configuration, st
out << minifi::docs::generateJsonSchema();
}
+void overridePropertiesFromCommandLine(const argparse::ArgumentParser& parser, const std::shared_ptr<minifi::Configure>& configure) {
+ const auto& properties = parser.get<std::vector<std::string>>("--property");
+ for (const auto& property : properties) {
+ auto property_key_and_value = utils::StringUtils::splitAndTrimRemovingEmpty(property, "=");
+ if (property_key_and_value.size() != 2) {
+ std::cerr << "Command line property must be defined in <key>=<value> format, invalid property: " << property << std::endl;
+ std::cerr << parser;
+ std::exit(1);
+ }
+ configure->set(property_key_and_value[0], property_key_and_value[1]);
+ }
+}
+
+void dumpDocsIfRequested(const argparse::ArgumentParser& parser, const std::shared_ptr<minifi::Configure>& configure) {
+ if (!parser.is_used("--docs")) {
+ return;
+ }
+ const auto& docs_params = parser.get<std::vector<std::string>>("--docs");
+ if (utils::file::create_dir(docs_params[0]) != 0) {
+ std::cerr << "Working directory doesn't exist and cannot be created: " << docs_params[0] << std::endl;
+ exit(1);
Review Comment:
Updated in 8e64616285aa944ae5e35bece9c07b025f737577
##########
minifi_main/MiNiFiMain.cpp:
##########
@@ -134,7 +135,93 @@ void writeJsonSchema(const std::shared_ptr<minifi::Configure> &configuration, st
out << minifi::docs::generateJsonSchema();
}
+void overridePropertiesFromCommandLine(const argparse::ArgumentParser& parser, const std::shared_ptr<minifi::Configure>& configure) {
+ const auto& properties = parser.get<std::vector<std::string>>("--property");
+ for (const auto& property : properties) {
+ auto property_key_and_value = utils::StringUtils::splitAndTrimRemovingEmpty(property, "=");
+ if (property_key_and_value.size() != 2) {
+ std::cerr << "Command line property must be defined in <key>=<value> format, invalid property: " << property << std::endl;
+ std::cerr << parser;
+ std::exit(1);
+ }
+ configure->set(property_key_and_value[0], property_key_and_value[1]);
+ }
+}
+
+void dumpDocsIfRequested(const argparse::ArgumentParser& parser, const std::shared_ptr<minifi::Configure>& configure) {
+ if (!parser.is_used("--docs")) {
+ return;
+ }
+ const auto& docs_params = parser.get<std::vector<std::string>>("--docs");
+ if (utils::file::create_dir(docs_params[0]) != 0) {
+ std::cerr << "Working directory doesn't exist and cannot be created: " << docs_params[0] << std::endl;
+ exit(1);
+ }
+
+ std::cout << "Dumping docs to " << docs_params[0] << std::endl;
+ if (docs_params.size() > 1) {
+ auto path = std::filesystem::path(docs_params[1]);
+ if (std::filesystem::exists(path) && !std::filesystem::is_regular_file(path)) {
+ std::cerr << "PROCESSORS.md target path exists, but it is not a regular file: " << path << std::endl;
+ exit(1);
+ }
+ auto dir = path.parent_path();
+ if (dir == docs_params[0]) {
+ std::cerr << "Target file should be out of the working directory: " << dir << std::endl;
+ exit(1);
+ }
+ std::ofstream outref(docs_params[1]);
+ dumpDocs(configure, docs_params[0], outref);
+ } else {
+ dumpDocs(configure, docs_params[0], std::cout);
+ }
+ exit(0);
+}
+
+void writeSchemaIfRequested(const argparse::ArgumentParser& parser, const std::shared_ptr<minifi::Configure>& configure) {
+ if (!parser.is_used("--schema")) {
+ return;
+ }
+ const auto& schema_path = parser.get("--schema");
+ if (std::filesystem::exists(schema_path) && !std::filesystem::is_regular_file(schema_path)) {
+ std::cerr << "JSON schema target path already exists, but it is not a regular file: " << schema_path << std::endl;
+ exit(1);
+ }
+
+ auto parent_dir = std::filesystem::path(schema_path).parent_path();
+ if (utils::file::create_dir(parent_dir) != 0) {
+ std::cerr << "JSON schema parent directory doesn't exist and cannot be created: " << parent_dir << std::endl;
+ exit(1);
+ }
+ std::cout << "Writing json schema to " << schema_path << std::endl;
+ std::ofstream schema_file{schema_path};
+ writeJsonSchema(configure, schema_file);
+ std::exit(0);
Review Comment:
Updated in 8e64616285aa944ae5e35bece9c07b025f737577
--
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-minifi-cpp] szaszm commented on a diff in pull request #1638: MINIFICPP-1774 Set properties from command line arguments
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1638:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638#discussion_r1312114968
##########
controller/CMakeLists.txt:
##########
@@ -18,6 +18,9 @@
#
cmake_minimum_required(VERSION 3.16)
+if (CMAKE_VERSION VERSION_GREATER_EQUAL 3.24)
+ cmake_policy(SET CMP0135 NEW) # policy to set the timestamps of extracted contents to the time of extraction
+endif()
Review Comment:
After #1624, we might as well change the required minimum version here as well, and get rid of the if statement. Anyone who compiles minifi will have CMake 3.24+ anyway.
--
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-minifi-cpp] lordgamez commented on a diff in pull request #1638: MINIFICPP-1774 Set properties from command line arguments
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1638:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638#discussion_r1328383550
##########
LICENSE:
##########
@@ -1250,60 +1250,6 @@ Redistribution and use in source and binary forms, with or without modification,
THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-This product bundles 'cxxopts' which is available under an MIT license.
-
-Copyright (c) 2014 Jarryd Beck
-
-Permission is hereby granted, free of charge, to any person obtaining a copy
-of this software and associated documentation files (the "Software"), to deal
-in the Software without restriction, including without limitation the rights
-to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
-copies of the Software, and to permit persons to whom the Software is
-furnished to do so, subject to the following conditions:
-
-The above copyright notice and this permission notice shall be included in
-all copies or substantial portions of the Software.
-
-THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
-IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
-FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
-AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
-LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
-OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
-THE SOFTWARE.
-
-This product bundles zlib which is available under the zlib license:
-
- (C) 1995-2017 Jean-loup Gailly and Mark Adler
Review Comment:
Good catch, updated in fedcf987b23f949b053a4cced79ef7b9fd2478e6
##########
controller/MiNiFiController.cpp:
##########
@@ -106,81 +109,108 @@ int main(int argc, char **argv) {
socket_data.ssl_context_service = getSSLContextService(configuration);
} catch(const minifi::Exception& ex) {
logger->log_error(ex.what());
- exit(1);
+ std::exit(1);
}
- cxxopts::Options options("MiNiFiController", "MiNiFi local agent controller");
- options.positional_help("[optional args]").show_positional_help();
-
- options.add_options()
- ("h,help", "Shows Help")
- ("host", "Specifies connecting host name", cxxopts::value<std::string>())
- ("port", "Specifies connecting host port", cxxopts::value<int>())
- ("stop", "Shuts down the provided component", cxxopts::value<std::vector<std::string>>())
- ("start", "Starts provided component", cxxopts::value<std::vector<std::string>>())
- ("l,list", "Provides a list of connections or processors", cxxopts::value<std::string>())
- ("c,clear", "Clears the associated connection queue", cxxopts::value<std::vector<std::string>>())
- ("getsize", "Reports the size of the associated connection queue", cxxopts::value<std::vector<std::string>>())
- ("updateflow", "Updates the flow of the agent using the provided flow file", cxxopts::value<std::string>())
- ("getfull", "Reports a list of full connections")
- ("jstack", "Returns backtraces from the agent")
- ("manifest", "Generates a manifest for the current binary")
- ("noheaders", "Removes headers from output streams");
+ argparse::ArgumentParser argument_parser("Apache MiNiFi C++ Controller", minifi::AgentBuild::VERSION);
+ argument_parser.add_argument("--host").metavar("HOSTNAME")
+ .help("Specifies connecting host name");
+ argument_parser.add_argument("--port")
+ .metavar("PORT")
+ .help("Specifies connecting host port")
+ .scan<'d', int>();
+ argument_parser.add_argument("--stop")
+ .metavar("COMPONENT")
+ .nargs(argparse::nargs_pattern::at_least_one)
+ .help("Shuts down the provided components");
+ argument_parser.add_argument("--start")
+ .metavar("COMPONENT")
+ .nargs(argparse::nargs_pattern::at_least_one)
+ .help("Starts provided components");
+ argument_parser.add_argument("-l", "--list")
+ .action([](const std::string& value) {
+ static const std::vector<std::string> choices = {"components", "connections"};
+ if (ranges::contains(choices, value)) {
+ return value;
+ }
Review Comment:
Updated in fedcf987b23f949b053a4cced79ef7b9fd2478e6
--
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-minifi-cpp] lordgamez commented on a diff in pull request #1638: MINIFICPP-1774 Set properties from command line arguments
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1638:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638#discussion_r1328383317
##########
controller/CMakeLists.txt:
##########
@@ -18,6 +18,9 @@
#
cmake_minimum_required(VERSION 3.16)
+if (CMAKE_VERSION VERSION_GREATER_EQUAL 3.24)
+ cmake_policy(SET CMP0135 NEW) # policy to set the timestamps of extracted contents to the time of extraction
+endif()
Review Comment:
Updated in fedcf987b23f949b053a4cced79ef7b9fd2478e6
--
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-minifi-cpp] fgerlits closed pull request #1638: MINIFICPP-1774 Set properties from command line arguments
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits closed pull request #1638: MINIFICPP-1774 Set properties from command line arguments
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638
--
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-minifi-cpp] szaszm commented on a diff in pull request #1638: MINIFICPP-1774 Set properties from command line arguments
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1638:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638#discussion_r1303419741
##########
minifi_main/MiNiFiMain.cpp:
##########
Review Comment:
Could we add a `--help` option that just prints help and exists with 0?
--
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-minifi-cpp] lordgamez commented on a diff in pull request #1638: MINIFICPP-1774 Set properties from command line arguments
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1638:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638#discussion_r1307378898
##########
LICENSE:
##########
@@ -3416,3 +3416,13 @@ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.
+
+This product bundles 'argparse' which is available under The MIT License.
Review Comment:
One feature I cannot see in `cxxopts` that we used here with the `--docs` options is to provide variable number of parameters for an option. In `argparse` I could define `nargs` with 1 or 2 options depending on if the user wants to specify a second parameter for the PROCESSORS.md output. We also want to reuse the `--property` multiple times which looks to be a feature of `cxxopts` 3.0, but currently we are using 2.2. It seems that `cxxopts` is more lightweight, but with less features than `argparse` which looks to be in parity with the features of the python's argparse lib. I only saw the `ArgumentParser` class used in `EncryptConfig` so I replaced that, but I suppose it would be better to have a single argument parser in the overall codebase. What do you think, should we replace `cxxopts` in minificontroller too or can we use `cxxopts` with our options somehow?
--
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-minifi-cpp] adamdebreceni commented on a diff in pull request #1638: MINIFICPP-1774 Set properties from command line arguments
Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1638:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638#discussion_r1307394810
##########
LICENSE:
##########
@@ -3416,3 +3416,13 @@ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.
+
+This product bundles 'argparse' which is available under The MIT License.
Review Comment:
if the newest `cxxopts` provides something similar to `nargs` you mentioned we could go for an update, it has a more recent release and more stars, if not, `argparse` could replace `cxxopts`, but the important thing IMO is to not have two thirdparty arg parsing libraries
--
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-minifi-cpp] lordgamez commented on a diff in pull request #1638: MINIFICPP-1774 Set properties from command line arguments
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1638:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638#discussion_r1304042639
##########
minifi_main/MiNiFiMain.cpp:
##########
Review Comment:
It does call `exit(0)` with the default `--help` implementation here: https://github.com/p-ranav/argparse/blob/b0930ab0288185815d6dc67af59de7014a6272f7/include/argparse/argparse.hpp#L1094C13-L1094C13
--
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-minifi-cpp] lordgamez commented on a diff in pull request #1638: MINIFICPP-1774 Set properties from command line arguments
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1638:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638#discussion_r1308934754
##########
LICENSE:
##########
@@ -3416,3 +3416,13 @@ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.
+
+This product bundles 'argparse' which is available under The MIT License.
Review Comment:
I replaced `cxxopts` with `argparse` in 9c933309f6184abbc748625308d5c41b43a31f37
--
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-minifi-cpp] lordgamez commented on a diff in pull request #1638: MINIFICPP-1774 Set properties from command line arguments
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1638:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638#discussion_r1303911507
##########
minifi_main/MiNiFiMain.cpp:
##########
Review Comment:
The `-h/--help` option is provided by the `argparse` library by default.
--
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-minifi-cpp] szaszm commented on a diff in pull request #1638: MINIFICPP-1774 Set properties from command line arguments
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1638:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638#discussion_r1303939812
##########
minifi_main/MiNiFiMain.cpp:
##########
Review Comment:
How does it exit? Does it call `exit(0)` internally? (That seems to be a bit of an overreach.)
--
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-minifi-cpp] lordgamez commented on a diff in pull request #1638: MINIFICPP-1774 Set properties from command line arguments
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1638:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638#discussion_r1303946082
##########
minifi_main/MiNiFiMain.cpp:
##########
Review Comment:
It exits with exit code 0, seems to work the same way as python's argparse, here's the more information: https://github.com/p-ranav/argparse#default-arguments This is the default functionality but can be overriden and handled differently.
--
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-minifi-cpp] lordgamez commented on a diff in pull request #1638: MINIFICPP-1774 Set properties from command line arguments
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1638:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638#discussion_r1303946082
##########
minifi_main/MiNiFiMain.cpp:
##########
Review Comment:
It exits with exit code 0, seems to work the same way as python's argparse, here's the more information: https://github.com/p-ranav/argparse#default-arguments
--
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-minifi-cpp] fgerlits commented on a diff in pull request #1638: MINIFICPP-1774 Set properties from command line arguments
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #1638:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638#discussion_r1327179105
##########
controller/MiNiFiController.cpp:
##########
@@ -106,81 +109,108 @@ int main(int argc, char **argv) {
socket_data.ssl_context_service = getSSLContextService(configuration);
} catch(const minifi::Exception& ex) {
logger->log_error(ex.what());
- exit(1);
+ std::exit(1);
}
- cxxopts::Options options("MiNiFiController", "MiNiFi local agent controller");
- options.positional_help("[optional args]").show_positional_help();
-
- options.add_options()
- ("h,help", "Shows Help")
- ("host", "Specifies connecting host name", cxxopts::value<std::string>())
- ("port", "Specifies connecting host port", cxxopts::value<int>())
- ("stop", "Shuts down the provided component", cxxopts::value<std::vector<std::string>>())
- ("start", "Starts provided component", cxxopts::value<std::vector<std::string>>())
- ("l,list", "Provides a list of connections or processors", cxxopts::value<std::string>())
- ("c,clear", "Clears the associated connection queue", cxxopts::value<std::vector<std::string>>())
- ("getsize", "Reports the size of the associated connection queue", cxxopts::value<std::vector<std::string>>())
- ("updateflow", "Updates the flow of the agent using the provided flow file", cxxopts::value<std::string>())
- ("getfull", "Reports a list of full connections")
- ("jstack", "Returns backtraces from the agent")
- ("manifest", "Generates a manifest for the current binary")
- ("noheaders", "Removes headers from output streams");
+ argparse::ArgumentParser argument_parser("Apache MiNiFi C++ Controller", minifi::AgentBuild::VERSION);
+ argument_parser.add_argument("--host").metavar("HOSTNAME")
+ .help("Specifies connecting host name");
+ argument_parser.add_argument("--port")
+ .metavar("PORT")
+ .help("Specifies connecting host port")
+ .scan<'d', int>();
+ argument_parser.add_argument("--stop")
+ .metavar("COMPONENT")
+ .nargs(argparse::nargs_pattern::at_least_one)
+ .help("Shuts down the provided components");
+ argument_parser.add_argument("--start")
+ .metavar("COMPONENT")
+ .nargs(argparse::nargs_pattern::at_least_one)
+ .help("Starts provided components");
+ argument_parser.add_argument("-l", "--list")
+ .action([](const std::string& value) {
+ static const std::vector<std::string> choices = {"components", "connections"};
+ if (ranges::contains(choices, value)) {
+ return value;
+ }
Review Comment:
Very minor, but this is shorter and cheaper:
```suggestion
if (ranges::contains(std::array{"components", "connections"}, value)) {
return value;
}
```
##########
LICENSE:
##########
@@ -1250,60 +1250,6 @@ Redistribution and use in source and binary forms, with or without modification,
THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-This product bundles 'cxxopts' which is available under an MIT license.
-
-Copyright (c) 2014 Jarryd Beck
-
-Permission is hereby granted, free of charge, to any person obtaining a copy
-of this software and associated documentation files (the "Software"), to deal
-in the Software without restriction, including without limitation the rights
-to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
-copies of the Software, and to permit persons to whom the Software is
-furnished to do so, subject to the following conditions:
-
-The above copyright notice and this permission notice shall be included in
-all copies or substantial portions of the Software.
-
-THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
-IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
-FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
-AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
-LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
-OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
-THE SOFTWARE.
-
-This product bundles zlib which is available under the zlib license:
-
- (C) 1995-2017 Jean-loup Gailly and Mark Adler
Review Comment:
I don't think this should be deleted, since we still bundle `zlib`
--
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-minifi-cpp] szaszm commented on a diff in pull request #1638: MINIFICPP-1774 Set properties from command line arguments
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1638:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1638#discussion_r1308837404
##########
minifi_main/MiNiFiMain.cpp:
##########
Review Comment:
I still find it strange, but it works fine in our case, so I'm closing this thread.
--
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