[NIFI-12778] manage remote ports#8433
Conversation
scottyaslan
force-pushed
the
NIFI-12778
branch
2 times, most recently
from
237dce4 to
8a1c156
Compare
mcgilman
left a comment
mcgilman
left a comment
There was a problem hiding this comment.
Looking good. Thanks for the PR @scottyaslan! I've noted a few things below.
...web-frontend/src/main/nifi/src/app/pages/flow-designer/service/manage-remote-port.service.ts
Outdated
Show resolved
Hide resolved
...ork/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-designer/state/flow/index.ts
Show resolved
Hide resolved
...in/nifi/src/app/pages/flow-designer/state/manage-remote-ports/manage-remote-ports.effects.ts
Outdated
Show resolved
Hide resolved
...in/nifi/src/app/pages/flow-designer/state/manage-remote-ports/manage-remote-ports.effects.ts
Outdated
Show resolved
Hide resolved
...ain/nifi/src/app/pages/flow-designer/ui/manage-remote-ports/manage-remote-ports.component.ts
Show resolved
Hide resolved
...ain/nifi/src/app/pages/flow-designer/ui/manage-remote-ports/manage-remote-ports.component.ts
Outdated
Show resolved
Hide resolved
...in/nifi/src/app/pages/flow-designer/state/manage-remote-ports/manage-remote-ports.effects.ts
Outdated
Show resolved
Hide resolved
...bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/state/shared/index.ts
Outdated
Show resolved
Hide resolved
scottyaslan
force-pushed
the
NIFI-12778
branch
from
8a1c156 to
1f6918e
Compare
mcgilman
left a comment
mcgilman
left a comment
There was a problem hiding this comment.
Thanks for the updates @scottyaslan! I noticed a few more items below.
...n/nifi/src/app/pages/flow-designer/ui/manage-remote-ports/manage-remote-ports.component.html
Outdated
Show resolved
Hide resolved
...eb-frontend/src/main/nifi/src/app/pages/flow-designer/service/canvas-context-menu.service.ts
Show resolved
Hide resolved
| on(loadRemotePortsSuccess, (state, { response }) => ({ | ||
| ...state, | ||
| ports: response.ports, | ||
| loadedTimestamp: response.rpg.component.flowRefreshed || '', |
There was a problem hiding this comment.
If the remote flow isn't current this will prevent the port listing from loading as it's used in the condition that shows the skeleton loader.
Also, flowRefreshed won't update each time the listing reloads.
There was a problem hiding this comment.
Now the loadedTimestamp in the store is calculated from the users browser, the flow configuration timeOffset, and the about timezone values.
...in/nifi/src/app/pages/flow-designer/state/manage-remote-ports/manage-remote-ports.effects.ts
Show resolved
Hide resolved
...ork/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/flow-designer/state/flow/index.ts
Outdated
Show resolved
Hide resolved
...pp/pages/flow-designer/ui/manage-remote-ports/edit-remote-port/edit-remote-port.component.ts
Outdated
Show resolved
Hide resolved
...in/nifi/src/app/pages/flow-designer/state/manage-remote-ports/manage-remote-ports.effects.ts
Outdated
Show resolved
Hide resolved
| compressed: new FormControl(request.entity.useCompression || false), | ||
| count: new FormControl(request.entity.batchSettings.count || ''), | ||
| size: new FormControl(request.entity.batchSettings.size || ''), | ||
| duration: new FormControl(request.entity.batchSettings.duration || '') |
There was a problem hiding this comment.
We need to use or and null coalescing carefully. || will evaluate to the right side if the left side evaluates to falsy. This includes null/undefined and things like 0 or empty string. If any of the settings are 0 or empty string it will evaluate to the right side. If we used ?? here instead, we would only evaluate to the ride side when the left side is null or undefined.
There was a problem hiding this comment.
I think I addressed this along with the way these values are displayed in the main table listing. Please have another look.
| .manage-remote-ports-table { | ||
| .listing-table { | ||
| .fa.fa-warning { | ||
| color: $canvas-accent-palette-A200; |
There was a problem hiding this comment.
@mcgilman I wasn't sure what color (if any) this icon should be? Red seems too strong and is used throughout nifi to represent an error or invalid. However, the application does not seem to mix using this fa-warning icon with a red color but rather these fa-warning icons typically have a yellow color. I am also fine if you think we just want to leave the icon with no extra colors applied and just let it be the default .listing-table .icon color.
mcgilman
left a comment
mcgilman
left a comment
There was a problem hiding this comment.
Thanks for the updates @scottyaslan! I noticed a few more items below.
...ain/nifi/src/app/pages/flow-designer/ui/manage-remote-ports/manage-remote-ports.component.ts
Outdated
Show resolved
Hide resolved
...fi-web-frontend/src/main/nifi/src/app/pages/flow-designer/state/manage-remote-ports/index.ts
Show resolved
Hide resolved
...ain/nifi/src/app/pages/flow-designer/ui/manage-remote-ports/manage-remote-ports.component.ts
Show resolved
Hide resolved
...ain/nifi/src/app/pages/flow-designer/ui/manage-remote-ports/manage-remote-ports.component.ts
Show resolved
Hide resolved
...ain/nifi/src/app/pages/flow-designer/ui/manage-remote-ports/manage-remote-ports.component.ts
Show resolved
Hide resolved
scottyaslan
force-pushed
the
NIFI-12778
branch
from
7828c04 to
5fbc8f7
Compare
| } | ||
|
|
||
| formatDuration(entity: PortSummary): string { | ||
| if (!this.isSizeBlank(entity)) { |
There was a problem hiding this comment.
| if (!this.isSizeBlank(entity)) { | |
| if (!this.isDurationBlank(entity)) { |
mcgilman
left a comment
mcgilman
left a comment
There was a problem hiding this comment.
Thanks for the updates @scottyaslan! Will merge once CI completes.
2 participants
NIFI-12778