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/05/29 17:09:51 UTC
[GitHub] nifi-minifi-cpp pull request #347: MINIFIPP-514 Incorporated regex peroperty...
GitHub user achristianson opened a pull request:
https://github.com/apache/nifi-minifi-cpp/pull/347
MINIFIPP-514 Incorporated regex peroperty validation information into agent manifest
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-514
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi-minifi-cpp/pull/347.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 #347
----
commit f49baba21de54105806d9236ae8f21f204ef55ff
Author: Andrew I. Christianson <an...@...>
Date: 2018-05-24T19:34:29Z
MINIFICPP-500 Incorporate mutually-exclusive property metadata into agent information
commit fa5718f4d6561562dc6302398c432c9a0acb1d9e
Author: Andrew I. Christianson <an...@...>
Date: 2018-05-29T17:08:56Z
MINIFIPP-514 Incorporated regex peroperty validation information into agent manifest
----
---
[GitHub] nifi-minifi-cpp issue #347: MINIFIPP-514 Incorporated regex property validat...
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/347
It looks like this went in before we had outstanding items tended to, the least of which would be an issue which I do not currently see in JIRA. We should either get those tasks captured/logged or revert this commit until we can reach a general consensus.
---
[GitHub] nifi-minifi-cpp issue #347: MINIFIPP-514 Incorporated regex property validat...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/347
@achristianson and @apiri I don't believe the requests made in these pull requests were ever responded to. It includes this PR and two others that are all the same commits, just layered. I'm okay pushing those requests to another ticket as long as that ticket is a blocker for this release. I don't want to push to a ticket just for the reason that "it's not in the scope of this ticket." and that get forgotten. IMO my request for these tickets was a pretty important request that wasn't responded to.
---
[GitHub] nifi-minifi-cpp issue #347: MINIFIPP-514 Incorporated regex property validat...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/347
@achristianson I've seen that when there are port collisions in tests. I've made https://issues.apache.org/jira/browse/MINIFICPP-518 and will take a look.
---
[GitHub] nifi-minifi-cpp issue #347: MINIFIPP-514 Incorporated regex property validat...
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/347
@achristianson would you mind rebasing this one last time so we can merge it in?
---
[GitHub] nifi-minifi-cpp issue #347: MINIFIPP-514 Incorporated regex property validat...
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/347
@phrocker Please elaborate what exactly your issues are. As you can see from 20 days ago, all concerns were addressed to the best of my knowledge.
---
[GitHub] nifi-minifi-cpp pull request #347: MINIFIPP-514 Incorporated regex peroperty...
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/347#discussion_r191842547
--- Diff: extensions/http-curl/processors/InvokeHTTP.cpp ---
@@ -65,7 +65,13 @@ core::Property InvokeHTTP::FollowRedirects("Follow Redirects", "Follow HTTP redi
core::Property InvokeHTTP::AttributesToSend("Attributes to Send", "Regular expression that defines which attributes to send as HTTP"
" headers in the request. If not defined, no attributes are sent as headers.",
"");
-core::Property InvokeHTTP::SSLContext("SSL Context Service", "The SSL Context Service used to provide client certificate information for TLS/SSL (https) connections.", "");
+core::Property InvokeHTTP::SSLContext("SSL Context Service",
+ "The SSL Context Service used to provide client certificate "
+ "information for TLS/SSL (https) connections.",
+ "",
+ false,
+ {},
+ {{"Remote URL", "^http:.*$"}});
--- End diff --
@apiri mentioned his interpretation of this. let's change this and use a builder instead. This is difficult to read.
---
[GitHub] nifi-minifi-cpp issue #347: MINIFIPP-514 Incorporated regex property validat...
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/347
Build fail looks unrelated.
---
[GitHub] nifi-minifi-cpp issue #347: MINIFIPP-514 Incorporated regex property validat...
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/347
Fixed.
---
[GitHub] nifi-minifi-cpp pull request #347: MINIFIPP-514 Incorporated regex property ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi-minifi-cpp/pull/347
---
[GitHub] nifi-minifi-cpp issue #347: MINIFIPP-514 Incorporated regex property validat...
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/347
@phrocker I missed those when they got folded up by the GitHub view. Apologies for that.
---
[GitHub] nifi-minifi-cpp pull request #347: MINIFIPP-514 Incorporated regex peroperty...
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/347#discussion_r191843838
--- Diff: extensions/http-curl/processors/InvokeHTTP.cpp ---
@@ -65,7 +65,13 @@ core::Property InvokeHTTP::FollowRedirects("Follow Redirects", "Follow HTTP redi
core::Property InvokeHTTP::AttributesToSend("Attributes to Send", "Regular expression that defines which attributes to send as HTTP"
" headers in the request. If not defined, no attributes are sent as headers.",
"");
-core::Property InvokeHTTP::SSLContext("SSL Context Service", "The SSL Context Service used to provide client certificate information for TLS/SSL (https) connections.", "");
+core::Property InvokeHTTP::SSLContext("SSL Context Service",
+ "The SSL Context Service used to provide client certificate "
+ "information for TLS/SSL (https) connections.",
+ "",
+ false,
+ {},
+ {{"Remote URL", "^http:.*$"}});
--- End diff --
This is confusing because the PR is based on MINIFICPP-500 mutually-exclusive. This PR is for MINIFICPP-514 so that Remote URL mutually-exclusive property is correct.
A URL regex could get complex, but in this case we're just checking for HTTP protocol as per RFC3986 (which is exclusive of this SSL property, for mutual-exclusivity as defined by MINIFICPP-467 E.g. "If I set certificates in InvokeHTTP, I shouldn’t allow http as the target protocol.").
Should it use a controller service? Probably. That's out of scope for this ticket but I agree with a new ticket for that.
All these notes apply for MINIFICPP-500, not MINIFICPP-514.
---
[GitHub] nifi-minifi-cpp issue #347: MINIFIPP-514 Incorporated regex property validat...
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/347
Looks like some conflict(s) cropped up. Taking a look.
---
[GitHub] nifi-minifi-cpp pull request #347: MINIFIPP-514 Incorporated regex peroperty...
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/347#discussion_r191844258
--- Diff: extensions/http-curl/processors/InvokeHTTP.cpp ---
@@ -65,7 +65,13 @@ core::Property InvokeHTTP::FollowRedirects("Follow Redirects", "Follow HTTP redi
core::Property InvokeHTTP::AttributesToSend("Attributes to Send", "Regular expression that defines which attributes to send as HTTP"
" headers in the request. If not defined, no attributes are sent as headers.",
"");
-core::Property InvokeHTTP::SSLContext("SSL Context Service", "The SSL Context Service used to provide client certificate information for TLS/SSL (https) connections.", "");
+core::Property InvokeHTTP::SSLContext("SSL Context Service",
+ "The SSL Context Service used to provide client certificate "
+ "information for TLS/SSL (https) connections.",
+ "",
+ false,
+ {},
+ {{"Remote URL", "^http:.*$"}});
--- End diff --
To clarify, the regex is just checking for positive incidence of HTTP protocol, nothing more. It isn't there to validate a URL (again this is very confusing due to the incidental git basing).
---
[GitHub] nifi-minifi-cpp pull request #347: MINIFIPP-514 Incorporated regex peroperty...
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/347#discussion_r191842946
--- Diff: extensions/http-curl/processors/InvokeHTTP.cpp ---
@@ -65,7 +65,13 @@ core::Property InvokeHTTP::FollowRedirects("Follow Redirects", "Follow HTTP redi
core::Property InvokeHTTP::AttributesToSend("Attributes to Send", "Regular expression that defines which attributes to send as HTTP"
" headers in the request. If not defined, no attributes are sent as headers.",
"");
-core::Property InvokeHTTP::SSLContext("SSL Context Service", "The SSL Context Service used to provide client certificate information for TLS/SSL (https) connections.", "");
+core::Property InvokeHTTP::SSLContext("SSL Context Service",
+ "The SSL Context Service used to provide client certificate "
+ "information for TLS/SSL (https) connections.",
+ "",
+ false,
+ {},
+ {{"Remote URL", "^http:.*$"}});
--- End diff --
would be in favor of that
---
[GitHub] nifi-minifi-cpp pull request #347: MINIFIPP-514 Incorporated regex peroperty...
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/347#discussion_r191551385
--- Diff: extensions/http-curl/processors/InvokeHTTP.cpp ---
@@ -65,7 +65,13 @@ core::Property InvokeHTTP::FollowRedirects("Follow Redirects", "Follow HTTP redi
core::Property InvokeHTTP::AttributesToSend("Attributes to Send", "Regular expression that defines which attributes to send as HTTP"
" headers in the request. If not defined, no attributes are sent as headers.",
"");
-core::Property InvokeHTTP::SSLContext("SSL Context Service", "The SSL Context Service used to provide client certificate information for TLS/SSL (https) connections.", "");
+core::Property InvokeHTTP::SSLContext("SSL Context Service",
+ "The SSL Context Service used to provide client certificate "
+ "information for TLS/SSL (https) connections.",
+ "",
+ false,
+ {},
+ {{"Remote URL", "^http:.*$"}});
--- End diff --
shouldn't this be https? I also think that URL regexes tend to be much more complicated than .*
---
[GitHub] nifi-minifi-cpp pull request #347: MINIFIPP-514 Incorporated regex peroperty...
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/347#discussion_r191551798
--- Diff: extensions/http-curl/processors/InvokeHTTP.cpp ---
@@ -65,7 +65,13 @@ core::Property InvokeHTTP::FollowRedirects("Follow Redirects", "Follow HTTP redi
core::Property InvokeHTTP::AttributesToSend("Attributes to Send", "Regular expression that defines which attributes to send as HTTP"
" headers in the request. If not defined, no attributes are sent as headers.",
"");
-core::Property InvokeHTTP::SSLContext("SSL Context Service", "The SSL Context Service used to provide client certificate information for TLS/SSL (https) connections.", "");
+core::Property InvokeHTTP::SSLContext("SSL Context Service",
+ "The SSL Context Service used to provide client certificate "
+ "information for TLS/SSL (https) connections.",
+ "",
+ false,
+ {},
+ {{"Remote URL", "^http:.*$"}});
--- End diff --
Actually, is this option part of SSL Context Service? Seems like the wrong variable name for the property.
---
[GitHub] nifi-minifi-cpp issue #347: MINIFIPP-514 Incorporated regex property validat...
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/347
@apiri This was merged on the basis of "I'll provide an implicit agreement to move forward." MINIFICPP-577 has been created to refactor the Property constructor to reduce the # of arguments. I don't see that it was actually clarified in the conversation as to what the issue was, but that is my best guess. Pleas update ticket if incorrect.
---
[GitHub] nifi-minifi-cpp pull request #347: MINIFIPP-514 Incorporated regex property ...
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/347#discussion_r205109263
--- Diff: extensions/http-curl/processors/InvokeHTTP.cpp ---
@@ -65,7 +65,13 @@ core::Property InvokeHTTP::FollowRedirects("Follow Redirects", "Follow HTTP redi
core::Property InvokeHTTP::AttributesToSend("Attributes to Send", "Regular expression that defines which attributes to send as HTTP"
" headers in the request. If not defined, no attributes are sent as headers.",
"");
-core::Property InvokeHTTP::SSLContext("SSL Context Service", "The SSL Context Service used to provide client certificate information for TLS/SSL (https) connections.", "");
+core::Property InvokeHTTP::SSLContext("SSL Context Service",
+ "The SSL Context Service used to provide client certificate "
+ "information for TLS/SSL (https) connections.",
+ "",
+ false,
+ {},
+ {{"Remote URL", "^http:.*$"}});
--- End diff --
I see this comment chain can be a bit confusing -- there were offline discussions that make it a bit worse. My request for a separate ticket was that this and other PRs have made the usage of these classes very difficult to read and use. I believe that it's bad enough that it warrants resolution in this ticket or subsequent work. To make code worse and then fix it doesn't seem correct; however, I'm okay with a subsequent ticket. I'm -1 with my opinion merit especially since there was no discussion of this. I think Aldrin and others can disagree and move forward with their +1s, through which I'll provide an implicit agreement to move forward -- I'm also open to the possibility that I'm completely wrong so let's actually have that discussion. My goal is not to stop this but there was no discussion regarding how this and others have made the code difficult to read and use.
---
[GitHub] nifi-minifi-cpp pull request #347: MINIFIPP-514 Incorporated regex peroperty...
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/347#discussion_r191844972
--- Diff: extensions/http-curl/processors/InvokeHTTP.cpp ---
@@ -65,7 +65,13 @@ core::Property InvokeHTTP::FollowRedirects("Follow Redirects", "Follow HTTP redi
core::Property InvokeHTTP::AttributesToSend("Attributes to Send", "Regular expression that defines which attributes to send as HTTP"
" headers in the request. If not defined, no attributes are sent as headers.",
"");
-core::Property InvokeHTTP::SSLContext("SSL Context Service", "The SSL Context Service used to provide client certificate information for TLS/SSL (https) connections.", "");
+core::Property InvokeHTTP::SSLContext("SSL Context Service",
+ "The SSL Context Service used to provide client certificate "
+ "information for TLS/SSL (https) connections.",
+ "",
+ false,
+ {},
+ {{"Remote URL", "^http:.*$"}});
--- End diff --
I'm afraid I'm going to disagree. I'm -1 on this PR until that issue is addressed. Completely happy with it being a separate ticket.
---