You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/11/18 20:22:16 UTC
[GitHub] [nifi] NissimShiman opened a new pull request, #6685: NIFI-10608 Copying Process Group now includes non-processor referenced controller services
NissimShiman opened a new pull request, #6685:
URL: https://github.com/apache/nifi/pull/6685
<!-- Licensed to the Apache Software Foundation (ASF) under one or more -->
<!-- contributor license agreements. See the NOTICE file distributed with -->
<!-- this work for additional information regarding copyright ownership. -->
<!-- The ASF licenses this file to You under the Apache License, Version 2.0 -->
<!-- (the "License"); you may not use this file except in compliance with -->
<!-- the License. You may obtain a copy of the License at -->
<!-- http://www.apache.org/licenses/LICENSE-2.0 -->
<!-- Unless required by applicable law or agreed to in writing, software -->
<!-- distributed under the License is distributed on an "AS IS" BASIS, -->
<!-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -->
<!-- See the License for the specific language governing permissions and -->
<!-- limitations under the License. -->
# Summary
[NIFI-10608](https://issues.apache.org/jira/browse/NIFI-10608)
Copying a processor group used to miss controller services if controller service had not been referenced to a processor.
This resolves that.
To show fix:
create new PG -> right click -> Configure -> Controller Sevices -> Add any controller service (AvroReader for example)
copy PG
copied PG -> right click -> Configure -> Controller Services -> (new controller service is now listed/ i.e. AvroReader will be listed)
Prior to this the new controller service/Avro Reader would not be listed for the copied PG.
This same issue was also present when making templates (i.e. the template would miss controller service that had not been referenced to a processor). This is now resolved for templates as well.
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [ ] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [ ] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [ ] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [ ] Pull Request based on current revision of the `main` branch
- [ ] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [ ] Build completed using `mvn clean install -P contrib-check`
- [ ] JDK 8
- [ ] JDK 11
- [ ] JDK 17
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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] NissimShiman commented on pull request #6685: NIFI-10608 Copying Process Group now includes non-processor referenced controller services
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on PR #6685:
URL: https://github.com/apache/nifi/pull/6685#issuecomment-1359998180
@joewitt I think that could be done. i.e. a unit test for this in SnippetUtilsTest ( https://github.com/apache/nifi/blob/main/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/test/java/org/apache/nifi/web/util/SnippetUtilsTest.java )
although as @markobean noted, this kind of gets into the more overarching question of testing in general for copying nifi components and if we want to address that.
(i.e. I am a little concerned this could lead to scope creep. I happened to come across this when testing/voting for apache nifi 1.18.0 RC4, so I wanted to close the loop with this, but this discussion is revealing that testing copying in general is an area that needs to be developed that I am hoping not to be led into at the moment :))
So to answer the question, yes, a unit test can be done for this, but we need to be OK with the general lack of copying related unit/integration tests that may now be more readily noticeable by having this one added.
--
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] joewitt commented on pull request #6685: NIFI-10608 Copying Process Group now includes non-processor referenced controller services
Posted by GitBox <gi...@apache.org>.
joewitt commented on PR #6685:
URL: https://github.com/apache/nifi/pull/6685#issuecomment-1359584735
Are there existing unit tests you could add to which would show the fix has the intended result?
--
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] NissimShiman commented on pull request #6685: NIFI-10608 Copying Process Group now includes non-processor referenced controller services
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on PR #6685:
URL: https://github.com/apache/nifi/pull/6685#issuecomment-1370103926
Thank you very much @mattyb149 and @markobean for looking at this!
--
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] markobean commented on pull request #6685: NIFI-10608 Copying Process Group now includes non-processor referenced controller services
Posted by GitBox <gi...@apache.org>.
markobean commented on PR #6685:
URL: https://github.com/apache/nifi/pull/6685#issuecomment-1332496182
First, I was able to reproduce the problem with NiFi 1.18.0. I created a process group with an unreferenced controller service. Then, I copy/pasted the PG. The new PG did not contain the unreferenced controller service.
Next, I rebased this PR branch to current main because I was having build issues on my platform with source code from pre-1.19.0. Performed a full build with unit tests successfully.
I installed NiFi and attempted to reproduce the copy/paste problem. The pasted PG correctly contained the unreferenced controller service.
Also, I noted the same problem exists when creating a template in 1.18.0; an unreferenced CS is not in the template. Confirmed by usage and by inspecting exported template XML. With this bug fix, the template correctly includes the unreferenced CS.
Thanks for the fix @NissimShiman !
+1
--
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] markobean commented on pull request #6685: NIFI-10608 Copying Process Group now includes non-processor referenced controller services
Posted by GitBox <gi...@apache.org>.
markobean commented on PR #6685:
URL: https://github.com/apache/nifi/pull/6685#issuecomment-1359924493
Reminder @joewitt - I did not merge because I am not a committer.
As far as unit tests, it may be reasonable to add, but this feels more like an integration testing option. And might begin to go down the rabbit hole. If we add a test for this, do we also add a test to ensure all processors are copied with the process group? child process groups? funnels? etc.
@NissimShiman If you think adding a unit test for the copy operation is reasonable, is it also reasonable to have each component type tested?
--
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] joewitt commented on pull request #6685: NIFI-10608 Copying Process Group now includes non-processor referenced controller services
Posted by GitBox <gi...@apache.org>.
joewitt commented on PR #6685:
URL: https://github.com/apache/nifi/pull/6685#issuecomment-1359879872
Huh - i just noticed @markobean did review this. @markobean any reason you didn't merge it?
Got ya on tests @NissimShiman . Do you think you could create a reasonable test with reasonable effort or is this code hard to test reliably?
--
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] mattyb149 closed pull request #6685: NIFI-10608 Copying Process Group now includes non-processor referenced controller services
Posted by GitBox <gi...@apache.org>.
mattyb149 closed pull request #6685: NIFI-10608 Copying Process Group now includes non-processor referenced controller services
URL: https://github.com/apache/nifi/pull/6685
--
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] NissimShiman commented on pull request #6685: NIFI-10608 Copying Process Group now includes non-processor referenced controller services
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on PR #6685:
URL: https://github.com/apache/nifi/pull/6685#issuecomment-1359870811
@joewitt That is a fair point. For some reason this area of code is lightly tested (i.e. there are no unit tests for the method this is called in. And that goes for all the parent methods of this as well)
--
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] joewitt commented on pull request #6685: NIFI-10608 Copying Process Group now includes non-processor referenced controller services
Posted by GitBox <gi...@apache.org>.
joewitt commented on PR #6685:
URL: https://github.com/apache/nifi/pull/6685#issuecomment-1360012142
sorry about that Mark - i have been thinking ever since I saw that note before you were. My mistake.
I think if tests existed you should be able to add to them. Creating tests where there are none and then having to deal with the existing untested bits is a bit much yeah. I'm fine with this as is but I do think someone more familiar with this portion of the code should review/merge. Mark's review from a functional point of view should make this super fast for someone more familiar. If not...I'll merge it and call it good soon.
--
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] mattyb149 commented on pull request #6685: NIFI-10608 Copying Process Group now includes non-processor referenced controller services
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on PR #6685:
URL: https://github.com/apache/nifi/pull/6685#issuecomment-1370017990
+1 LGTM, thanks for the review Mark! I too verified the expected behavior. Thanks for the fix! Merging to main
--
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