[GitHub] nifi issue #2768: NIFI-5278: fixes JSON escaping of code parameter in Execut...
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2768
+1 LGTM, ran build with unit tests and tried a pyspark session with
`print('Hi')`, verified it fails before the fix and passes after. Thanks for
the fix! Merging to master
---
[GitHub] nifi issue #2768: NIFI-5278: fixes JSON escaping of code parameter in Execut...
Github user peter-toth commented on the issue: https://github.com/apache/nifi/pull/2768 @mattyb149 I've rebased this onto latest master. ---
[GitHub] nifi issue #2768: NIFI-5278: fixes JSON escaping of code parameter in Execut...
Github user mattyb149 commented on the issue: https://github.com/apache/nifi/pull/2768 @peter-toth can you rebase this against the latest master? Not sure if you'd worked on this and other Livy stuff at the same time, but there are now merge conflicts and I wasn't quite sure what to include from each. ---
[GitHub] nifi issue #2768: NIFI-5278: fixes JSON escaping of code parameter in Execut...
Github user mgaido91 commented on the issue: https://github.com/apache/nifi/pull/2768 LGTM ---
[GitHub] nifi issue #2768: NIFI-5278: fixes JSON escaping of code parameter in Execut...
Github user peter-toth commented on the issue: https://github.com/apache/nifi/pull/2768 @joewitt , thanks for the feedback. I've added Apache Commons Text to NOTICE of the nifi-livy-nar and nifi-assembly as you suggested. I checked that it does not bring in any new transitive dependency and also amended the existing test. ---
[GitHub] nifi issue #2768: NIFI-5278: fixes JSON escaping of code parameter in Execut...
Github user joewitt commented on the issue: https://github.com/apache/nifi/pull/2768 @peter-toth Since this is adding a new dependency (at least directly and possibly transitively) we'll at least need to make a License and Notice update. Can you please verify what the old dependencies that would be included are for impacted nar(s) and see what the difference is. The new artifacts need to be in the Nar(s) L&N as appropriate. For commons-text at least I suspect we need an entry in the spark-nar and possibly in the nifi-assembly notice as well. Please take a look to identify the gaps so we can get this included. To the other comment(s) from otto/marco it would be good, if there is already a unit test, to create one which shows this change. If there were not unit or integration tests I dont think you should be held to a higher standard than the original author was while you're trying to fix a bug. We can proceed without if so but we cannot proceed without L&N updates as needed. ---
[GitHub] nifi issue #2768: NIFI-5278: fixes JSON escaping of code parameter in Execut...
Github user ottobackwards commented on the issue: https://github.com/apache/nifi/pull/2768 Is there a test that should be created or updated for this change? ---
