-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NIFI-3785: Added feature to move controller services between process … #8965
base: main
Are you sure you want to change the base?
Conversation
@markap14 @exceptionfactory I've resubmitted this PR with the changes done to the new UI. The backend is the same as it was in the closed PR(#7734), but the UI changes were done against the new UI. |
Thanks for putting in the work to refactor this for the new UI @Freedom9339. |
Thanks for the mention @exceptionfactory! I'll have a look... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @Freedom9339! I've left a number of comments below that mostly appear to be from using the Enable/Disable Service dialog has a starting point. Since that dialog needs to be used for both Services in the Flow and in the Controller Settings there area a number of considerations there that are not needed here.
In addition to the items below, please ensure you run prettier and that this PR isn't introducing any new lint issues (I think there are currently 31 left that we're slowly trying to work through). From nifi/nifi-frontend/src/main/frontend
please run:
$ npx nx prettier:format
$ npx nx lint
I will defer to @exceptionfactory or @markap14 for a more thorough review of the back end changes.
const serviceId: string = request.id; | ||
request.processGroupFlow = this.flowService.getFlow(currentProcessGroupId); | ||
const moveDialogReference = this.dialog.open(MoveControllerService, { | ||
...XL_DIALOG, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is an XL Dialog needed here? It seems like the a smaller dialog may be more appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this to a LARGE_DIALOG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed this. It now extends all the way.
...ps/nifi/src/app/pages/flow-designer/state/controller-services/controller-services.effects.ts
Outdated
Show resolved
Hide resolved
...ps/nifi/src/app/pages/flow-designer/state/controller-services/controller-services.reducer.ts
Outdated
Show resolved
Hide resolved
@if (canConfigure(item)) { | ||
<button mat-menu-item (click)="moveClicked(item)"> | ||
<i class="fa fa-arrows primary-color mr-2"></i> | ||
Move | ||
</button> | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This table is used for both services in the Flow as well as services in the Controller Settings. This is why the ControllerServiceTable
is in the common
components. Since Services in the Controller Settings do not exist within a Process Group, this action is not be available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made the move option not show up in management controller services.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update here. The new Input()
supports function that returns whether the service can be moved. However, the implements are not service specific and always just return true
or false
. Can this Input()
be simplified into a boolean?
@Input() canMove: boolean = false;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this to just be a boolean.
.../ui/common/controller-service/move-controller-service/move-controller-service.component.html
Outdated
Show resolved
Hide resolved
...pp/ui/common/controller-service/move-controller-service/move-controller-service.component.ts
Outdated
Show resolved
Hide resolved
...pp/ui/common/controller-service/move-controller-service/move-controller-service.component.ts
Outdated
Show resolved
Hide resolved
nifi-frontend/src/main/frontend/apps/nifi/src/app/state/shared/index.ts
Outdated
Show resolved
Hide resolved
concatLatestFrom(() => this.store.select(selectCurrentProcessGroupId)), | ||
tap(([request, currentProcessGroupId]) => { | ||
const serviceId: string = request.id; | ||
request.processGroupFlow = this.flowService.getFlow(currentProcessGroupId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The state for the flow-designer
page is already loaded with the current process group. We should be able to simply use that instead of making another request here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current state did not store the entire flow. I did change this and I was able to use it here without an api call. However, with the implementation of disabled drop down menu process groups, I did need to 2 other api calls here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry. Yes you are correct. I thought we had stashed the entire processGroupFlow
in the service state as well but I see that only saved the breadcrumbs and parameter context details. Now we're saving the entire processGroupFlow
next to the breadcrumbs and parameter context details.
Do we only need the processGroupFlow
in order to get the child Process Groups for the Move dialog? If so, what are your thoughts only saving them in the store. Often times the flow can be very large and in this page we won't need most of those details.
Let me know if I missed something and we do need additional details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the idea, I hadn't thought of that. I removed the storing of the whole process group flow, and just saved an array with names and IDs.
.../common/controller-service/move-controller-service/move-controller-service.component.spec.ts
Outdated
Show resolved
Hide resolved
@mcgilman Just checking in. Any updates on this? |
Thanks for the ping. Sorry for the delay, I'll try to get some more eyes on this PR this week. |
@mcgilman Sorry the pinging again. Just want to make sure this doesn't fall through the cracks. |
I just pulled down the latest. After rebasing against current
I then tried running with the built application but I'm seeing this error in dev tools when attempting to open the
Once the PR is updated I should be able to get some more eyes on it quickly. |
9392904
to
9009d70
Compare
@mcgilman I fixed the issue caused by rebasing and pushed. Thank You! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the rebase! I've left some additional feedback below.
value: child.value | ||
}; | ||
|
||
const root = this.getProcessGroupById(currentProcessGroupEntity.component, child.value ?? ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getProcessGroupById
will return null for a couple different scenarios. When this happens calling processGroupContainsComponent
will error with the following. The unhandled error leaves the UI in a weird state because the overlay is active but the dialog isn't shown.
TypeError: Cannot read properties of null (reading 'contents')
at _MoveControllerService.processGroupContainsComponent (move-controller-service.component.ts:184:26)
at move-controller-service.component.ts:166:27
at Array.forEach (<anonymous>)
at _MoveControllerService.loadChildOptions (move-controller-service.component.ts:157:34)
at new _MoveControllerService (move-controller-service.component.ts:99:14)
at NodeInjectorFactory.MoveControllerService_Factory [as factory] (move-controller-service.component.ts:231:5)
at getNodeInjectable (core.mjs:6060:44)
at createRootComponent (core.mjs:17231:35)
at ComponentFactory.create (core.mjs:17091:29)
at ViewContainerRef2.createComponent (core.mjs:17498:47)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears this was happening when childProcessGroupOptions
contained more than one option. The recursive logic in getProcessGroupById
(and other methods with similar structure) isn't correct. When there are multiple children the following code
for (const pg of root.contents.processGroups) {
return this.getProcessGroupById(pg, processGroupId);
}
will return the result from the first child and not attempt to process any subsequent children.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed the recursion logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing the recursion issue. However getProcessGroupById
returns null in a couple scenarios. That response value is used as though it's guaranteed non-null. We should protect against this.
@@ -63,7 +66,8 @@ import { | |||
} from '../../../../state/property-verification/property-verification.selectors'; | |||
import { VerifyPropertiesRequestContext } from '../../../../state/property-verification'; | |||
import { BackNavigation } from '../../../../state/navigation'; | |||
import { NiFiCommon, Storage } from '@nifi/shared'; | |||
import { NiFiCommon, Storage, SelectOption, ComponentType, LARGE_DIALOG, SMALL_DIALOG, XL_DIALOG } from '@nifi/shared'; | |||
import { ComponentEntity } from './../flow/index'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import { ComponentEntity } from './../flow/index'; | |
import { ComponentEntity } from '../flow'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
import { ControllerServiceEntity, ParameterContextReferenceEntity } from '../../../../state/shared'; | ||
import { BreadcrumbEntity } from '../shared'; | ||
import { Revision } from './../../../../state/shared/index'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import { Revision } from './../../../../state/shared/index'; | |
import { Revision } from '../../../../state/shared'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
); | ||
|
||
const processGroups: SelectOption[] = []; | ||
if (request.breadcrumb != undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Under what conditions is request.breadcrumb
undefined
? I think the store has an initial value but even if that isn't the case we could update the concatLatestFrom
with the following:
this.store.select(selectBreadcrumb).pipe(isDefinedAndNotNull())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to only use a single request model, but I've created 2 and removed the need to declare it as breadcrumb?
const clone = Object.assign({}, request.request); | ||
clone.processGroupEntity = request.processGroupEntity; | ||
clone.childProcessGroupOptions = request.childProcessGroupOptions; | ||
clone.parentControllerServices = request.controllerServices; | ||
clone.breadcrumb = request.breadcrumb; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is a clone being created here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. Removed need to clone.
9009d70
to
5a86dbe
Compare
@mcgilman I've completed the change/fixes requested. Thank You! |
@mcgilman Sorry, I just realized my latest commit didn't go through. It's there now. |
c368f43
to
712413e
Compare
@mcgilman Any update on this? I just did a fresh rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @Freedom9339! I've left a few more comments below.
} | ||
|
||
getProcessGroupById(root: any, processGroupId: string): any { | ||
console.log('a'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was left in inadvertently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was. I've removed it.
value: child.value | ||
}; | ||
|
||
const root = this.getProcessGroupById(currentProcessGroupEntity.component, child.value ?? ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing the recursion issue. However getProcessGroupById
returns null in a couple scenarios. That response value is used as though it's guaranteed non-null. We should protect against this.
</mat-dialog-content> | ||
<mat-dialog-actions align="end"> | ||
<button mat-button mat-dialog-close>Cancel</button> | ||
<button type="button" color="primary" (click)="submitForm()" mat-button>Move</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This button should be disabled when the form is not valid. For instance, when only disabled options are present and the user cannot make a selection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The button is now disabled when there no valid options.
} | ||
|
||
submitForm() { | ||
if (this.moveControllerServiceForm.get('processGroups')?.value != 'Process Group') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the Submit button is disabled, I think we should be able to remove this check here as the form would never be submitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
<div> | ||
<div>Service</div> | ||
<div | ||
class="accent-color font-medium overflow-ellipsis overflow-hidden whitespace-nowrap" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class="accent-color font-medium overflow-ellipsis overflow-hidden whitespace-nowrap" | |
class="tertiary-color font-medium truncate" |
<form class="controller-service-move-form" [formGroup]="moveControllerServiceForm"> | ||
<mat-dialog-content> | ||
<div class="py-4 flex gap-x-3"> | ||
<div class="flex basis-2/3 flex-col gap-y-4 pr-4 overflow-hidden"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<div class="flex basis-2/3 flex-col gap-y-4 pr-4 overflow-hidden"> | |
<div class="flex basis-1/2 flex-col gap-y-4 pr-4 overflow-hidden"> |
Thoughts on splitting the dialog space, half for the Group selection and half for the References?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it looks better. Done.
</mat-form-field> | ||
</div> | ||
</div> | ||
<div class="flex basis-1/3 flex-col"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<div class="flex basis-1/3 flex-col"> | |
<div class="flex basis-1/2 flex-col"> |
…groups with the new UI
… component to flow-designer. Added disabling options that have a move conflict
…upflow in the state and just load the info needed.
712413e
to
275c18d
Compare
@mcgilman Thank you for the review. I've made the requested changes. Thank You. |
…groups with the new UI
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 there are any scope conflicts with referencing components, the move will fail displaying the reason.
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000
NIFI-00000
Pull Request Formatting
main
branchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-check
Licensing
LICENSE
andNOTICE
filesDocumentation