Skip to content
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: rbd mirroring promotion/demotion UI #46524

Closed
wants to merge 1 commit into from

Conversation

Pegonzal
Copy link
Contributor

@Pegonzal Pegonzal commented Jun 5, 2022

I'll do the rebase later, when the branches are merged.

Development of interface for rbd image promotion/demotion actions.

Having issues updating primary attribute for rbd image journal mode.

Updating rbd image snapshot mode, working as intended:

screen-capture.1.mp4

When updating rbd image journal mode, I'm getting errno 22 as a response. Video and image of the issue:

screen-capture.mp4

Screenshot from 2022-06-05 17-47-54

Fixes: https://tracker.ceph.com/issues/55860
Signed-off-by: Pedro Gonzalez Gomez pegonzal@redhat.com

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@Pegonzal Pegonzal requested a review from a team as a code owner June 5, 2022 17:04
@Pegonzal Pegonzal added this to In progress in Dashboard via automation Jun 5, 2022
@Pegonzal Pegonzal requested review from nSedrickm and epuertat and removed request for a team June 5, 2022 17:04
Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just a few comments over there.

Comment on lines 67 to 70
<span *ngIf="row.primary === true"
class="badge badge-info">primary</span>
<span *ngIf="row.primary === false"
class="badge badge-info">non-primary</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to add the i18n property, since Dashboard is officially internationalized. BTW Bootstrap has a primary/secondary style for the badges. The following example uses the ngIf-Else clause (the else is often skipped, but I like to highlight that only one value is displayed):

<span i18n *ngIf="row.primary === true else secondary" class="badge badge-primary">
  primary
</span>
<ng-template #secondary>
  <span i18n class="badge badge-secondary">
    secondary
  </span>
</ng-template>

Or with ngClass and ICU plural:

<span i18n class="badge" [ngClass]="row.primary ? 'badge-primary' : 'badge-secondary'">
  {row.primary, plural, =1 {primary}, =0 {secondary}}
</span>

Comment on lines +198 to +213
const promoteAction: CdTableAction = {
permission: 'update',
icon: Icons.edit,
click: () => this.actionPrimary(true),
name: this.actionLabels.PROMOTE,
visible: () => (this.selection.first() != null && !this.selection.first().primary)
? true : false
};
const demoteAction: CdTableAction = {
permission: 'update',
icon: Icons.edit,
click: () => this.actionPrimary(false),
name: this.actionLabels.DEMOTE,
visible: () => (this.selection.first() != null && this.selection.first().primary)
? true : false
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than creating a couple of actions for promoting/demoting (imperative approach), what about adding a checkbox primary in the RBD edit form (declarative)? This is a question open to everyone, @ceph/dashboard ?

In this very case I may see the point for each approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it felt right to have these as actions, because I feel like a user that knows how this works would try to click on an action first. I prefer them as actions tbh but I understand why you want to go full declarative here.

@@ -502,6 +520,24 @@ export class RbdListComponent extends ListWithDetails implements OnInit {
});
}

actionPrimary(primary: boolean) {
const request = new RbdFormEditRequestModel();
request.primary = primary;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so in the back-end we're doing the declarative approach, right?

Dashboard automation moved this from In progress to Reviewer approved Jun 6, 2022
@github-actions
Copy link

github-actions bot commented Jun 7, 2022

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

Signed-off-by: Pedro Gonzalez Gomez <pegonzal@redhat.com>
@pereman2 pereman2 closed this Jun 17, 2022
@pereman2 pereman2 deleted the rbd-mirroring-snapshot-ui branch June 17, 2022 12:48
Dashboard automation moved this from Reviewer approved to Done Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Dashboard
  
Done
3 participants