You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by achristianson <gi...@git.apache.org> on 2018/01/08 17:19:16 UTC
[GitHub] nifi-minifi-cpp pull request #230: MINIFICPP-357 Added support for config YA...
GitHub user achristianson opened a pull request:
https://github.com/apache/nifi-minifi-cpp/pull/230
MINIFICPP-357 Added support for config YAML v3.
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:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with MINIFI-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 master)?
- [x] Is your initial contribution a single, squashed commit?
### For code changes:
- [x] 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)?
- [x] If applicable, have you updated the LICENSE file?
- [x] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [x] 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 travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/achristianson/nifi-minifi-cpp MINIFICPP-357
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi-minifi-cpp/pull/230.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #230
----
commit badddf33f9b5ce07d0ffdd887b434bd9e0217334
Author: Andy I. Christianson <an...@...>
Date: 2018-01-04T18:00:27Z
MINIFICPP-357 Added support for config YAML v3.
----
---
[GitHub] nifi-minifi-cpp issue #230: MINIFICPP-357 Added support for config YAML v3.
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/230
@achristianson think we're all good here. will get this merged in today. thanks
---
[GitHub] nifi-minifi-cpp issue #230: MINIFICPP-357 Added support for config YAML v3.
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/230
reviewing
---
[GitHub] nifi-minifi-cpp pull request #230: MINIFICPP-357 Added support for config YA...
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/230#discussion_r160269055
--- Diff: libminifi/include/core/yaml/YamlConfiguration.h ---
@@ -40,13 +40,18 @@ namespace core {
#define CONFIG_YAML_CONNECTIONS_KEY "Connections"
#define CONFIG_YAML_CONTROLLER_SERVICES_KEY "Controller Services"
#define CONFIG_YAML_REMOTE_PROCESS_GROUP_KEY "Remote Processing Groups"
+#define CONFIG_YAML_REMOTE_PROCESS_GROUP_KEY_V3 "Remote Process Groups"
#define CONFIG_YAML_PROVENANCE_REPORT_KEY "Provenance Reporting"
class YamlConfiguration : public FlowConfiguration {
public:
- explicit YamlConfiguration(std::shared_ptr<core::Repository> repo, std::shared_ptr<core::Repository> flow_file_repo, std::shared_ptr<core::ContentRepository> content_repo,
- std::shared_ptr<io::StreamFactory> stream_factory, std::shared_ptr<Configure> configuration, const std::string path = DEFAULT_FLOW_YAML_FILE_NAME)
+ explicit YamlConfiguration(std::shared_ptr<core::Repository> repo,
--- End diff --
Will fix.
---
[GitHub] nifi-minifi-cpp pull request #230: MINIFICPP-357 Added support for config YA...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/230#discussion_r160268671
--- Diff: libminifi/include/core/yaml/YamlConfiguration.h ---
@@ -40,13 +40,18 @@ namespace core {
#define CONFIG_YAML_CONNECTIONS_KEY "Connections"
#define CONFIG_YAML_CONTROLLER_SERVICES_KEY "Controller Services"
#define CONFIG_YAML_REMOTE_PROCESS_GROUP_KEY "Remote Processing Groups"
+#define CONFIG_YAML_REMOTE_PROCESS_GROUP_KEY_V3 "Remote Process Groups"
#define CONFIG_YAML_PROVENANCE_REPORT_KEY "Provenance Reporting"
class YamlConfiguration : public FlowConfiguration {
public:
- explicit YamlConfiguration(std::shared_ptr<core::Repository> repo, std::shared_ptr<core::Repository> flow_file_repo, std::shared_ptr<core::ContentRepository> content_repo,
- std::shared_ptr<io::StreamFactory> stream_factory, std::shared_ptr<Configure> configuration, const std::string path = DEFAULT_FLOW_YAML_FILE_NAME)
+ explicit YamlConfiguration(std::shared_ptr<core::Repository> repo,
--- End diff --
Did you shorten the line length? I think we abide by 200 characters as per nifi.
---
[GitHub] nifi-minifi-cpp issue #230: MINIFICPP-357 Added support for config YAML v3.
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/230
@apiri sure thing.
---
[GitHub] nifi-minifi-cpp issue #230: MINIFICPP-357 Added support for config YAML v3.
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/230
Overall this looks okay but uncovered a wrong assumption in the MiNiFi Toolkit that does YAML transformations. I am looking into that a bit to verify a couple of things but can likely move this one ahead. Will follow up and thanks!
---
[GitHub] nifi-minifi-cpp pull request #230: MINIFICPP-357 Added support for config YA...
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/230#discussion_r160433655
--- Diff: libminifi/include/core/yaml/YamlConfiguration.h ---
@@ -40,13 +40,18 @@ namespace core {
#define CONFIG_YAML_CONNECTIONS_KEY "Connections"
#define CONFIG_YAML_CONTROLLER_SERVICES_KEY "Controller Services"
#define CONFIG_YAML_REMOTE_PROCESS_GROUP_KEY "Remote Processing Groups"
+#define CONFIG_YAML_REMOTE_PROCESS_GROUP_KEY_V3 "Remote Process Groups"
#define CONFIG_YAML_PROVENANCE_REPORT_KEY "Provenance Reporting"
class YamlConfiguration : public FlowConfiguration {
public:
- explicit YamlConfiguration(std::shared_ptr<core::Repository> repo, std::shared_ptr<core::Repository> flow_file_repo, std::shared_ptr<core::ContentRepository> content_repo,
- std::shared_ptr<io::StreamFactory> stream_factory, std::shared_ptr<Configure> configuration, const std::string path = DEFAULT_FLOW_YAML_FILE_NAME)
+ explicit YamlConfiguration(std::shared_ptr<core::Repository> repo,
--- End diff --
Updated my IDE to 200 and the auto-formatter still does it this way. TBH it is much more readable to have them on separate lines and conforms with the Google style guide as we follow. I don't think it's worth blocking the PR over.
---
[GitHub] nifi-minifi-cpp pull request #230: MINIFICPP-357 Added support for config YA...
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson closed the pull request at:
https://github.com/apache/nifi-minifi-cpp/pull/230
---
[GitHub] nifi-minifi-cpp issue #230: MINIFICPP-357 Added support for config YAML v3.
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/230
@achristianson just realized I had a misfire on the number pad. could you please close this PR manually? work was merged in c6f9dc628bd28e5788e2235bfe4b7f7c9000c941
---