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 2024/04/19 10:57:41 UTC
[PR] MINIFICPP-2295 Add controller service support for NiFi python processors [nifi-minifi-cpp]
lordgamez opened a new pull request, #1762:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1762
https://issues.apache.org/jira/browse/MINIFICPP-2295
-------------
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
Re: [PR] MINIFICPP-2295 Add controller service support for NiFi python processors [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #1762:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1762#discussion_r1584692377
##########
extensions/python/types/PyProcessContext.cpp:
##########
@@ -104,6 +107,30 @@ PyObject* PyProcessContext::getStateManager(PyProcessContext* self, PyObject*) {
return object::returnReference(context->getStateManager());
}
+PyObject* PyProcessContext::getControllerService(PyProcessContext* self, PyObject* args) {
+ auto context = self->process_context_.lock();
+ if (!context) {
+ PyErr_SetString(PyExc_AttributeError, "tried reading process context outside 'on_trigger'");
+ return nullptr;
+ }
+
+ const char* controller_service_name = nullptr;
+ const char* controller_service_type = nullptr;
+ if (!PyArg_ParseTuple(args, "s|s", &controller_service_name, &controller_service_type)) {
+ throw PyException();
+ }
+
+ if (auto controller_service = context->getControllerService(controller_service_name)) {
+ std::string controller_service_type_str = controller_service_type;
+ if (controller_service_type_str == "SSLContextService") {
+ auto ssl_ctx_service = std::dynamic_pointer_cast<controllers::SSLContextService>(controller_service);
+ return object::returnReference(std::weak_ptr(ssl_ctx_service));
+ }
+ }
Review Comment:
Since we only support `SSLContextService` for now, I think we should change the titles (commit, Jira, PR) to reflect this, eg. "Add SSLContextService support for NiFi python processors".
Also, we could log an error or warning here when the requested controller service type is something other than `SSLContextService`.
--
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
Re: [PR] MINIFICPP-2295 Add SSLContextService support for NiFi Python processors [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm closed pull request #1762: MINIFICPP-2295 Add SSLContextService support for NiFi Python processors
URL: https://github.com/apache/nifi-minifi-cpp/pull/1762
--
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
Re: [PR] MINIFICPP-2295 Add SSLContextService support for NiFi Python processors [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1762:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1762#discussion_r1590828391
##########
extensions/python/types/PyProcessContext.cpp:
##########
@@ -104,6 +107,30 @@ PyObject* PyProcessContext::getStateManager(PyProcessContext* self, PyObject*) {
return object::returnReference(context->getStateManager());
}
+PyObject* PyProcessContext::getControllerService(PyProcessContext* self, PyObject* args) {
+ auto context = self->process_context_.lock();
+ if (!context) {
+ PyErr_SetString(PyExc_AttributeError, "tried reading process context outside 'on_trigger'");
+ return nullptr;
+ }
+
+ const char* controller_service_name = nullptr;
+ const char* controller_service_type = nullptr;
+ if (!PyArg_ParseTuple(args, "s|s", &controller_service_name, &controller_service_type)) {
+ throw PyException();
+ }
+
+ if (auto controller_service = context->getControllerService(controller_service_name)) {
+ std::string controller_service_type_str = controller_service_type;
+ if (controller_service_type_str == "SSLContextService") {
+ auto ssl_ctx_service = std::dynamic_pointer_cast<controllers::SSLContextService>(controller_service);
+ return object::returnReference(std::weak_ptr(ssl_ctx_service));
+ }
+ }
Review Comment:
Updated the titles, unfortunately all python bindings use static function definitions so no logger object is present to log the errors, and we should not throw an exception here, only return a NONE object like NiFi does.
--
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
Re: [PR] MINIFICPP-2295 Add controller service support for NiFi python processors [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #1762:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1762#discussion_r1584692377
##########
extensions/python/types/PyProcessContext.cpp:
##########
@@ -104,6 +107,30 @@ PyObject* PyProcessContext::getStateManager(PyProcessContext* self, PyObject*) {
return object::returnReference(context->getStateManager());
}
+PyObject* PyProcessContext::getControllerService(PyProcessContext* self, PyObject* args) {
+ auto context = self->process_context_.lock();
+ if (!context) {
+ PyErr_SetString(PyExc_AttributeError, "tried reading process context outside 'on_trigger'");
+ return nullptr;
+ }
+
+ const char* controller_service_name = nullptr;
+ const char* controller_service_type = nullptr;
+ if (!PyArg_ParseTuple(args, "s|s", &controller_service_name, &controller_service_type)) {
+ throw PyException();
+ }
+
+ if (auto controller_service = context->getControllerService(controller_service_name)) {
+ std::string controller_service_type_str = controller_service_type;
+ if (controller_service_type_str == "SSLContextService") {
+ auto ssl_ctx_service = std::dynamic_pointer_cast<controllers::SSLContextService>(controller_service);
+ return object::returnReference(std::weak_ptr(ssl_ctx_service));
+ }
+ }
Review Comment:
Since we only support `SSLContextService` for now, I think we should change the titles (commit, Jira, PR) to reflect this, eg. "Add SSLContextService support for NiFi python processors".
--
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
Re: [PR] MINIFICPP-2295 Add controller service support for NiFi python processors [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1762:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1762#discussion_r1575788821
##########
extensions/python/PYTHON.md:
##########
@@ -102,10 +102,12 @@ NON_BLANK = 5
PORT = 6
```
+The last parameter of addProperty is the controller service type. If the property is a controller service, the controller service type should be provided. It should be the non-qualified type name of the controller service. Currently SSLContextService is the only controller service type supported.
+
```python
def onInitialize(processor):
processor.setSupportsDynamicProperties()
- processor.addProperty("property name","description","default value", True /*required*/, False /*expression language supported*/, False /*sensitive*/, 1 /*property type code*/)
+ processor.addProperty("property name", "description", "default value", True /*required*/, False /*expression language supported*/, False /*sensitive*/, 1 /*property type code*/, None /*controller service type name*/)
Review Comment:
Good point, I added comment for the arguments in 0b35f1cf419e8b57955f65f9192cf39b8d8a5fde. Due to the implementation of `PyProcessor::addProperty(PyProcessor* self, PyObject* args)` these are handled as positional arguments, so using keyword arguments would be misleading.
--
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
Re: [PR] MINIFICPP-2295 Add controller service support for NiFi python processors [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1762:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1762#discussion_r1575079441
##########
extensions/python/PYTHON.md:
##########
@@ -102,10 +102,12 @@ NON_BLANK = 5
PORT = 6
```
+The last parameter of addProperty is the controller service type. If the property is a controller service, the controller service type should be provided. It should be the non-qualified type name of the controller service. Currently SSLContextService is the only controller service type supported.
+
```python
def onInitialize(processor):
processor.setSupportsDynamicProperties()
- processor.addProperty("property name","description","default value", True /*required*/, False /*expression language supported*/, False /*sensitive*/, 1 /*property type code*/)
+ processor.addProperty("property name", "description", "default value", True /*required*/, False /*expression language supported*/, False /*sensitive*/, 1 /*property type code*/, None /*controller service type name*/)
Review Comment:
These `/**/` style comments are not valid python. I'm assuming this comes from the NiFi Python API. Otherwise I'd consider changing to keyword 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