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: Add UI for Cluster-wide OSD Flags configuration #22461

Merged
merged 1 commit into from Jul 4, 2018

Conversation

Projects
None yet
5 participants
@tspmelo
Contributor

tspmelo commented Jun 7, 2018

Signed-off-by: Tiago Melo <tmelo@suse.com>

After #21998

@votdev

This comment has been minimized.

Contributor

votdev commented Jun 8, 2018

auswahl_001

The button is 28px high, the rest of the buttons, fields, dropdowns in the datatable header are 30px high.

@tspmelo tspmelo force-pushed the tspmelo:wip-osd-flags-ui branch 2 times, most recently from 1d98d44 to 7290e92 Jun 15, 2018

@tspmelo tspmelo changed the title from [WIP] mgr/dashboard: Add UI for Cluster-wide OSD Flags configuration to mgr/dashboard: Add UI for Cluster-wide OSD Flags configuration Jun 18, 2018

@tspmelo tspmelo removed the DNM label Jun 18, 2018

@tspmelo

This comment has been minimized.

Contributor

tspmelo commented Jun 18, 2018

@votdev both buttons are 30px now and it seems the spacing problem also occurs here.

@tspmelo

This comment has been minimized.

Contributor

tspmelo commented Jun 18, 2018

Added the 3 flags that can't be changed. They will show up disabled on the modal.
image

@tspmelo tspmelo force-pushed the tspmelo:wip-osd-flags-ui branch from 7290e92 to 9fc6ff0 Jun 18, 2018

@Devp00l

Tests please :)

});
}
submitAction() {

This comment has been minimized.

@Devp00l

Devp00l Jun 19, 2018

Contributor

Please add some tests for this function

@@ -22,4 +22,12 @@ export class OsdService {
scrub(id, deep) {
return this.http.post(`${this.path}/${id}/scrub?deep=${deep}`, null);
}
getFlags() {
return this.http.get(`${this.path}/flags`);

This comment has been minimized.

@Devp00l

Devp00l Jun 19, 2018

Contributor

Tests please :)
(not just related to this line or file)

this.osdService.updateFlags(newFlags).subscribe(
(res) => {
console.log(res);

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Jun 19, 2018

Member

This console.log should be removed.

@LenzGr LenzGr added the feature label Jun 19, 2018

@ricardoasmarques

Please remove the console.log that I mentioned. Apart from that LGTM.

@tspmelo tspmelo force-pushed the tspmelo:wip-osd-flags-ui branch from 9fc6ff0 to fbe95a3 Jun 21, 2018

@tspmelo

This comment has been minimized.

Contributor

tspmelo commented Jun 22, 2018

@Devp00l, @ricardoasmarques: I have removed the console.log and added tests for the service and component.

@LenzGr LenzGr added the needs-rebase label Jun 22, 2018

@tspmelo tspmelo force-pushed the tspmelo:wip-osd-flags-ui branch from fbe95a3 to 58d8241 Jun 22, 2018

@tspmelo tspmelo removed the needs-rebase label Jun 22, 2018

@LenzGr LenzGr added the needs-rebase label Jun 26, 2018

@tspmelo tspmelo force-pushed the tspmelo:wip-osd-flags-ui branch from 58d8241 to 203bc82 Jun 26, 2018

@tspmelo tspmelo removed the needs-rebase label Jun 26, 2018

],
declarations: [OsdFlagsModalComponent],
providers: [BsModalRef]
});

This comment has been minimized.

@Devp00l

Devp00l Jun 27, 2018

Contributor

You have to pass to configureTestBed the second argument (useOldMethod) to reset test bed on every test even if developers tests this suite or else you will get an error on the test should finish running ngOnInit.

});
it('should run submitAction', () => {
let notiticationType: NotificationType;

This comment has been minimized.

@Devp00l

Devp00l Jun 27, 2018

Contributor

Please fix the typo s/notiticationType/notificationType/

httpTesting = TestBed.get(HttpTestingController);
fixture = TestBed.createComponent(OsdFlagsModalComponent);
component = fixture.componentInstance;
fixture.detectChanges();

This comment has been minimized.

@Devp00l

Devp00l Jun 27, 2018

Contributor

If you remove this line - you can remove line 70, 71, 72 and 92, 93, 94. But you have to add a component.ngOnInit() before line 53, but this line would increase the readability :)

@tspmelo tspmelo force-pushed the tspmelo:wip-osd-flags-ui branch from 203bc82 to 01ccca2 Jun 28, 2018

component.flags = getFlagsArray(component);
component.unknownFlags = ['foo'];

This comment has been minimized.

@Devp00l

Devp00l Jun 28, 2018

Contributor

Both submit tests share a equal code base as they both test the submit behavior. I would suggest putting them into their own describe. I just tested it and it works :) see here:

diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/osd/osd-flags-modal/osd-flags-modal.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/osd/osd-flags-modal/osd-flags-modal.component.spec.ts
index bb8e907..13a6660 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/osd/osd-flags-modal/osd-flags-modal.component.spec.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/osd/osd-flags-modal/osd-flags-modal.component.spec.ts
@@ -58,43 +58,41 @@ describe('OsdFlagsModalComponent', () => {
     expect(component.unknownFlags).toEqual(['foo']);
   });
 
-  it('should run submitAction', () => {
+  describe('test submit', function() {
     let notificationType: NotificationType;
-    const notificationService = TestBed.get(NotificationService);
-    spyOn(notificationService, 'show').and.callFake((type) => {
-      notificationType = type;
+    let notificationService: NotificationService;
+    let bsModalRef: BsModalRef;
+
+    beforeEach(() => {
+      notificationService = TestBed.get(NotificationService);
+      spyOn(notificationService, 'show').and.callFake((type) => {
+        notificationType = type;
+      });
+
+      bsModalRef = TestBed.get(BsModalRef);
+      spyOn(bsModalRef, 'hide').and.callThrough();
+      component.unknownFlags = ['foo'];
     });
 
-    const bsModalRef = TestBed.get(BsModalRef);
-    spyOn(bsModalRef, 'hide').and.callThrough();
+    it('should run submitAction', () => {
+      component.flags = getFlagsArray(component);
+      component.submitAction();
+      const req = httpTesting.expectOne('api/osd/flags');
+      req.flush(['purged_snapdirs', 'pause', 'foo']);
+      expect(req.request.body).toEqual({ flags: ['pause', 'purged_snapdirs', 'foo'] });
 
-    component.flags = getFlagsArray(component);
-    component.unknownFlags = ['foo'];
-
-    component.submitAction();
-    const req = httpTesting.expectOne('api/osd/flags');
-    req.flush(['purged_snapdirs', 'pause', 'foo']);
-    expect(req.request.body).toEqual({ flags: ['pause', 'purged_snapdirs', 'foo'] });
-
-    expect(notificationType).toBe(NotificationType.success);
-    expect(component.bsModalRef.hide).toHaveBeenCalledTimes(1);
-  });
-
-  it('should hide modal if request fails', () => {
-    const notificationService = TestBed.get(NotificationService);
-    spyOn(notificationService, 'show').and.returnValue(true);
-
-    const bsModalRef = TestBed.get(BsModalRef);
-    spyOn(bsModalRef, 'hide').and.callThrough();
+      expect(notificationType).toBe(NotificationType.success);
+      expect(component.bsModalRef.hide).toHaveBeenCalledTimes(1);
+    });
 
-    component.flags = [];
-    component.unknownFlags = ['foo'];
+    it('should hide modal if request fails', () => {
+      component.flags = [];
+      component.submitAction();
+      const req = httpTesting.expectOne('api/osd/flags');
+      req.flush([], { status: 500, statusText: 'failure' });
 
-    component.submitAction();
-    const req = httpTesting.expectOne('api/osd/flags');
-    req.flush([], { status: 500, statusText: 'failure' });
-
-    expect(notificationService.show).toHaveBeenCalledTimes(0);
-    expect(component.bsModalRef.hide).toHaveBeenCalledTimes(1);
+      expect(notificationService.show).toHaveBeenCalledTimes(0);
+      expect(component.bsModalRef.hide).toHaveBeenCalledTimes(1);
+    });
   });
 });

This comment has been minimized.

@tspmelo

tspmelo Jul 2, 2018

Contributor

Done.

this.bsModalRef.hide();
},
() => {
this.bsModalRef.hide();

This comment has been minimized.

@Devp00l

Devp00l Jun 28, 2018

Contributor

You should set a specific error description if it fails. So that the user knows something went wrong with the OSD settings. The generic message does not give any info that OSD flags couldn't be set.

This comment has been minimized.

@tspmelo

tspmelo Jul 2, 2018

Contributor

This will be done in a different PR.

(click)="configureClusterAction()">
<i class="fa fa-fw fa-cog"
aria-hidden="true"></i>
<ng-container i18n>Configure Cluster-wide OSD Flags</ng-container>

This comment has been minimized.

@Devp00l

Devp00l Jun 28, 2018

Contributor

How about Set instead of Configure as the actions a more boolean based.

This comment has been minimized.

@tspmelo

tspmelo Jul 2, 2018

Contributor

Done.

mgr/dashboard: Add UI for Cluster-wide OSD Flags configuration
Signed-off-by: Tiago Melo <tmelo@suse.com>

@tspmelo tspmelo force-pushed the tspmelo:wip-osd-flags-ui branch from 01ccca2 to 4d57690 Jul 2, 2018

@tspmelo

This comment has been minimized.

Contributor

tspmelo commented Jul 2, 2018

jenkins retest this please

@Devp00l

Devp00l approved these changes Jul 3, 2018

lgtm

@LenzGr LenzGr merged commit 9d8f378 into ceph:master Jul 4, 2018

5 checks passed

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
make check (arm64) make check succeeded
Details

@tspmelo tspmelo deleted the tspmelo:wip-osd-flags-ui branch Jul 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment