nifi icon indicating copy to clipboard operation
nifi copied to clipboard

NIFI-3785 Added feature to move a controller service to it's parent o…

Open Freedom9339 opened this issue 1 year ago • 6 comments

…r child process group

Summary

NIFI-3785 - Added an option on the controller services grid that moves a controller service to a specified process group. The controller service can be moved to the parent process group or a child process group. However, it can only be moved one level at a time. If the move is to a child process group, any processor that references the controller service that is outside the scope of the new process group will have the reference removed. Likewise, if the move is to a parent node, any controller service that is referenced by the controller service being moved and that is outside of the new scope will have that referenced removed.

Tracking

Issue Tracking

Pull Request Tracking

  • [X] Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • [X] Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • [X] Pull Request based on current revision of the main branch
  • [X] 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

  • [X] Build completed using mvn clean install -P contrib-check
    • [X] JDK 17

Licensing

  • [ ] New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • [ ] New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • [ ] Documentation formatting appears as expected in rendered files

Freedom9339 avatar Sep 15 '23 15:09 Freedom9339

I had trouble building, but once I rebased to main the issues resolved. Please rebase.

I installed NiFi and played with this feature. It worked as expected moving to parent, to a child process group where referencing components exist, to a child process group where referencing components do not exist (and references to the controller service were then removed automatically). All worked great.

The UX is intuitive and provides sufficient warning to the user that referencing components may be adversely affected by the controller service move.

I confirmed the API documentation contains the new move endpoint.

Full disclosure: I tested only using single-user-authorizer, so I have not yet tested possible authorization issues such as attempting to move the controller service to a process group where the user does not have modify permission.

See the couple comments on the code itself, but generally this looks great. Thanks @Freedom9339. This is a great feature which has been a long time coming!

markobean avatar Nov 03 '23 16:11 markobean

@markap14 Would mind taking a look at this PR? I do see you were the one who created the ticket.

Freedom9339 avatar Feb 15 '24 16:02 Freedom9339

Oh nice, thanks @Freedom9339 ! This is something that I've wanted for a while. I would definitely want to get a reviewer who knows the UI as well such as @mcgilman or @scottyaslan. I should have a chance to review the backend soon, probably some time this week. But it looks like the system tests failed and some of the unit tests failed as well. Unfortunately, it looks like the logs have already aged off. I'll see if I can reproduce the failures locally, but we'll definitely want to ensure that we get those cleaned up

markap14 avatar Feb 20 '24 22:02 markap14

@Freedom9339 you mind rebasing against main and pushing again? That should kick off the tests against the latest & greatest. I misread the github comments initially, thinking you pushed a rebased commit just a couple days ago. That explains why I can't kick off the build again

markap14 avatar Feb 20 '24 22:02 markap14

@markap14 I've rebased against main and pushed. Thank You!

Freedom9339 avatar Feb 21 '24 18:02 Freedom9339

@markap14 Thank you for the feedback. I've addressed all the changes and concerns provided. The process will now fail if there is a referencing component scope conflict and an error prompt will be shown listing the components that have a scope conflict.

Freedom9339 avatar Mar 05 '24 19:03 Freedom9339

@markap14 Good Morning, Have you had a chance to look at the changes?

Freedom9339 avatar Apr 15 '24 13:04 Freedom9339