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

mgr/dashboard: Asynchronous tasks (frontend) #20962

Merged
merged 1 commit into from Apr 12, 2018

Conversation

Projects
None yet
8 participants
@ricardoasmarques
Copy link
Member

ricardoasmarques commented Mar 19, 2018

This PR is the front-end part of PR 20870.

This PR includes the infrastructure to deal with asynchronous tasks in the front-end and a visual representation of all executing and most recently finished asynchronous tasks:

screenshot from 2018-03-19 14-43-11

Signed-off-by: Ricardo Marques rimarques@suse.com

@ricardoasmarques ricardoasmarques requested review from tspmelo , Devp00l and rjfd Mar 19, 2018

@ricardoasmarques ricardoasmarques changed the title mgr/dashboard: Asynchronous tasks front-end mgr/dashboard: Asynchronous tasks (front-end) Mar 19, 2018

@ricardoasmarques ricardoasmarques changed the title mgr/dashboard: Asynchronous tasks (front-end) mgr/dashboard: Asynchronous tasks (frontend) Mar 19, 2018

@ricardoasmarques ricardoasmarques changed the title mgr/dashboard: Asynchronous tasks (frontend) [DNM] mgr/dashboard: Asynchronous tasks (frontend) Mar 19, 2018

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-task-manager-frontend branch 2 times, most recently from abc7b21 to 575ac28 Mar 20, 2018

How to deal with asynchronous tasks in the front-end?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

All executing and most recently finished asynchronous tasks are displayed on the

This comment has been minimized.

@callithea

callithea Mar 28, 2018

Member

Suggestion: "running" instead of "executing".

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Mar 28, 2018

Author Member

"Executing" is used in many places (including the UI), are you suggesting to change it everywhere or only in the documentation?

This comment has been minimized.

@callithea

callithea Mar 29, 2018

Member

IMO "running tasks" is more fitting in this context, so maybe it's worth changing it everywhere.

This comment has been minimized.

@rjfd

rjfd Mar 29, 2018

Contributor

Changing "executing" to "running" is not that easy, it implies creating a new PR to change backend code that uses "executing" as name variables, dictionary keys, and message strings.
I don't think it's worth the effort.

This comment has been minimized.

@p-na

p-na Mar 29, 2018

Contributor

"running tasks" is less ambiguous than "executing tasks", IMHO. In the context of the provided screen shot though, I don't think anyone would get confused about "Executing", but in this text I'd definitely prefer to read "running tasks".

This comment has been minimized.

@rjfd

rjfd Mar 29, 2018

Contributor

@p-na does that mean that @ricardoasmarques should only rename in the documentation?

This comment has been minimized.

@callithea

callithea Mar 29, 2018

Member

Yes, I think @p-na is talking about the documentation here.
And what about changing "EXECUTING" to "RUNNING" (just the title in the list of tasks) - that wouldn't be a big deal, would it?

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Mar 29, 2018

Author Member

IMO we should use the same name everywhere.

@callithea @p-na are you ok if we merge the "EXECUTING" label, and create a separate PR in the future to rename everything from "executing" to "running" (including code variables)?

This comment has been minimized.

@callithea

callithea Mar 29, 2018

Member

@ricardoasmarques: I'm fine with that, thank you!

messages for each task on ``TaskManagerMessageService.messages``.
This messages can make use of the task metadata to provide more personalized messages.

When submiting an asynchronous task, the developer should provide a callback

This comment has been minimized.

@callithea

callithea Mar 28, 2018

Member

/s/submiting/submitting/

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Mar 28, 2018

Author Member

Fixed

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-task-manager-frontend branch 2 times, most recently from 51a36e4 to cb2d9b2 Mar 28, 2018

@ricardoasmarques ricardoasmarques changed the title [DNM] mgr/dashboard: Asynchronous tasks (frontend) mgr/dashboard: Asynchronous tasks (frontend) Mar 28, 2018

@ricardoasmarques ricardoasmarques removed the DNM label Mar 28, 2018

@ricardoasmarques

This comment has been minimized.

Copy link
Member Author

ricardoasmarques commented Mar 28, 2018

The backend part of this PR is now merged.

@tspmelo @rjfd @Devp00l This PR is now rebased and ready for review.

} else {
iconIndex = 0;
}
this.icon = icons[iconIndex];

This comment has been minimized.

@tspmelo

tspmelo Apr 2, 2018

Contributor

This will fail when iconIndex is 4.
maybe use if (this.executingTasks.length > 0) iconIndex = (iconIndex + 1) % icons.length

This comment has been minimized.

@ricardoasmarques
@Devp00l

Devp00l approved these changes Apr 9, 2018

Copy link
Contributor

Devp00l left a comment

lgtm

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-task-manager-frontend branch from cb2d9b2 to 9be8303 Apr 9, 2018

</tr>
<tr>
<td colspan="3" class="text-right">
<span class="date">{{ executingTask.begin_time | date:"yyyy-MM-dd HH:mm:ss" }}</span></td>

This comment has been minimized.

@tspmelo

tspmelo Apr 10, 2018

Contributor

You should use CdDatePipe here.

</tr>
<tr>
<td colspan="2" class="text-right">
<span class="date">{{ finishedTask.end_time | date:"yyyy-MM-dd HH:mm:ss" }}</span>

This comment has been minimized.

@tspmelo

tspmelo Apr 10, 2018

Contributor

You should use CdDatePipe here.

}
for (const finishedTask of this.finishedTasks) {
finishedTask.description = this.taskManagerMessageService.getDescription(finishedTask);
if (!finishedTask.success || !finishedTask.ret_value.success) {

This comment has been minimized.

@tspmelo

tspmelo Apr 10, 2018

Contributor

Are both always defined? In case one of them is undefined this will always return true.

private taskManagerMessageService: TaskManagerMessageService) {
}

notify(finishedTask: FinishedTask) {

This comment has been minimized.

@tspmelo

tspmelo Apr 10, 2018

Contributor

Maybe this method should be moved into the new notification service, if this is merged after that PR.

This comment has been minimized.

@tspmelo

tspmelo Apr 11, 2018

Contributor

NotificationService has been merged. You can now move this method to that service.


constructor() { }

getSuccessMessage(finishedTask: FinishedTask) {

This comment has been minimized.

@tspmelo

tspmelo Apr 10, 2018

Contributor

I think we should check if there is a value for the finishedTask.name key.
Otherwise if some async task method is implemented in the backend this will result in exceptions on the frontend.

@@ -0,0 +1,45 @@
::ng-deep .popover-content {

This comment has been minimized.

@tspmelo

tspmelo Apr 11, 2018

Contributor

Most of this style rules are now part of popover.scss.

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-task-manager-frontend branch 2 times, most recently from c66f41c to 781d829 Apr 11, 2018

@ricardoasmarques

This comment has been minimized.

Copy link
Member Author

ricardoasmarques commented Apr 11, 2018

@tspmelo I've addressed all your comments

@@ -82,4 +85,15 @@ export class NotificationService {
break;
}
}

notify(finishedTask: FinishedTask, success: boolean = true) {

This comment has been minimized.

@tspmelo

tspmelo Apr 11, 2018

Contributor

Could you rename the method to be easier to identify, something like notifyTask.

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Apr 11, 2018

Author Member

Done

// Callback that will be invoked after task is finished
(finishedTask: FinishedTask) => {
// Will display a notification message (success or error)
this.taskManagerNotificationService.notify(finishedTask, finishedTask.ret_value.success);

This comment has been minimized.

@tspmelo

tspmelo Apr 11, 2018

Contributor

This file needs to be updated to reflect the latest changes.

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Apr 11, 2018

Author Member

Done

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-task-manager-frontend branch from 781d829 to aa640a9 Apr 11, 2018

@votdev

votdev approved these changes Apr 12, 2018

@LenzGr LenzGr added this to the mimic milestone Apr 12, 2018

@LenzGr LenzGr added the feature label Apr 12, 2018

mgr/dashboard: Asynchronous tasks front-end
Signed-off-by: Ricardo Marques <rimarques@suse.com>

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-task-manager-frontend branch from aa640a9 to e21d9b1 Apr 12, 2018

@LenzGr LenzGr merged commit 1de0753 into ceph:master Apr 12, 2018

4 of 5 checks passed

make check (arm64) make check failed
Details
Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment