Changes to support multiple appliances #114

danielreed · 2017-08-01T22:17:11Z

OVClient will have a pool of connections now (a Map of appliance addr
to connection)

Each of the existing clients now will have to handle gathering
responses from all connections or individual connections.

The oneview-configuration.json takes a list of hosts now.

Issue #10

OVClient will have a pool of connections now (a Map of appliance addr to connection) Each of the existing clients now will have to handle gathering responses from all connections or individual connections. The oneview-configuration.json takes a list of hosts now. Issue #10

jesseolsen

Some comments, but if you are fine with it after reading through the comments, then I approve....

jesseolsen · 2017-08-01T22:31:45Z

src/listener/alerts-listener.js

-    });
-  }
-}
+/*


Files look identical. DOS vs. Unix line endings?

Yes just changing line endings to Unix, it looks weird in the diff.

jesseolsen · 2017-08-01T22:33:00Z

src/listener/dashboard-listener.js

+  }
+
+  ShowOneViewDashboard(msg) {
+    this.transform.send(msg, "Oh I'm sorry the dashboard is broken!");


Is it currently broken, or just with your change? TBD later on, with a follow-on commit?

Its broken for now. Will need to be fixed in a separate review.

jesseolsen · 2017-08-01T22:33:19Z

src/listener/dashboard-listener.js

+  ShowOneViewDashboard(msg) {
+    this.transform.send(msg, "Oh I'm sorry the dashboard is broken!");
+
+    // this.transform.send(msg, "Ok " + msg.message.user.name + ", I am going to generate your dashboard. This might take a little while.\nFor a more comprehensive view, see " + this.transform.hyperlink("https://" + this.client.host + "/#/dashboard", "Dashboard"));


TBD? Or should these lines be removed?

Going to leave these for now. We will need them later when we fix it.

jesseolsen · 2017-08-01T22:43:44Z

src/listener/server-profile-templates.js

-    let templateHyperlink = nameAndHyperlink.hyperlink;
-    this.transform.text(msg, msg.message.user.name + " I am going to fix the compliance issues for the profile template " + this.transform.hyperlink(templateHyperlink, templateName) + ".  Are you sure you want to do this? (@" + this.robot.name + " yes/@" + this.robot.name + " no)");
+    let nameAndHyperlink = getDeviceNameAndHyperLink(host + "/rest/server-profile-templates/" + msg.templateId);
+    this.transform.text(msg, msg.message.user.name + " I am going to fix the compliance issues for the profile template " + this.transform.hyperlink(nameAndHyperlink.hyperlink, nameAndHyperlink.deviceName) + ".  Are you sure you want to do this? (@" + this.robot.name + " yes/@" + this.robot.name + " no)");


Do you want to add: "... on host " + host + ". Are you sure...?"

Hmm...good question. Not sure what to do about this now. We may have to think about changing lots of our messaging now.

jesseolsen · 2017-08-01T22:49:51Z

src/listener/server-profile-templates.js

-    let templateHyperlink = nameAndHyperlink.hyperlink;
-    this.transform.text(msg, msg.message.user.name + " I am going to fix the compliance issues for the profile template " + this.transform.hyperlink(templateHyperlink, templateName) + ".  Are you sure you want to do this? (@" + this.robot.name + " yes/@" + this.robot.name + " no)");
+    let nameAndHyperlink = getDeviceNameAndHyperLink(host + "/rest/server-profile-templates/" + msg.templateId);
+    this.transform.text(msg, msg.message.user.name + " I am going to fix the compliance issues for the profile template " + this.transform.hyperlink(nameAndHyperlink.hyperlink, nameAndHyperlink.deviceName) + ".  Are you sure you want to do this? (@" + this.robot.name + " yes/@" + this.robot.name + " no)");


Again, could add ... for host X....

jesseolsen · 2017-08-01T23:08:57Z

src/oneview-sdk/notifications.js

-        queue.subscribe({ack: true}, ::emitter.onMessage);
-      });
-    }
+    this.robot.logger.info('Connected to SCMB, waiting for messages');


2 sentences

jesseolsen · 2017-08-01T23:11:55Z

src/oneview.js

-    robot.logger.info('Logged into OV appliance.');
+
+  client.login(false).then(() => {
+    robot.logger.info('Logged into OV appliance(s).');


OV --> OneView.
Also, would be nice to say "appliance" or "appliances" vs. "appliance(s)"

That does sound better.

jesseolsen · 2017-08-01T23:12:19Z

src/ov-brain.js

-          resourcesMap.set(sh.uri, {name: sh.name, hyperlink: sh.hyperlink, model: sh.model});
-          robot.logger.info('Found server hardware: (Name: ' + sh.name + ', Serial Number: ' + sh.serialNumber + ', URI: ' + sh.uri + ')');
+        for (let member of res.members) {
+          let uri = url.parse(member.hyperlink).hostname + '' + member.uri;


Remove + ''

jesseolsen · 2017-08-01T23:12:50Z

src/ov-brain.js

-          resourcesMap.set(spt.uri, {name: spt.name, hyperlink: spt.hyperlink, model: undefined});
-          robot.logger.info('Found server profile template: (Name: ' + spt.name + ', URI: ' + spt.uri + ')');
+        for (let member of res.members) {
+          let uri = url.parse(member.hyperlink).hostname + '' + member.uri;


Remove + ''

jesseolsen · 2017-08-01T23:13:14Z

src/ov-brain.js

+        let auth = client.getAuthToken(host);
+
+        let model = message.resource.model;
+        let uri = host + '' + message.resource.uri;


danielreed

I'll update this PR to address your feedback.

danielreed · 2017-08-02T01:54:16Z

src/listener/alerts-listener.js

-    });
-  }
-}
+/*


Yes just changing line endings to Unix, it looks weird in the diff.

danielreed · 2017-08-02T01:57:24Z

src/listener/dashboard-listener.js

+  }
+
+  ShowOneViewDashboard(msg) {
+    this.transform.send(msg, "Oh I'm sorry the dashboard is broken!");


Its broken for now. Will need to be fixed in a separate review.

danielreed · 2017-08-02T01:58:46Z

src/listener/dashboard-listener.js

+  ShowOneViewDashboard(msg) {
+    this.transform.send(msg, "Oh I'm sorry the dashboard is broken!");
+
+    // this.transform.send(msg, "Ok " + msg.message.user.name + ", I am going to generate your dashboard. This might take a little while.\nFor a more comprehensive view, see " + this.transform.hyperlink("https://" + this.client.host + "/#/dashboard", "Dashboard"));


Going to leave these for now. We will need them later when we fix it.

danielreed · 2017-08-02T02:02:06Z

src/listener/server-profile-templates.js

-    let templateHyperlink = nameAndHyperlink.hyperlink;
-    this.transform.text(msg, msg.message.user.name + " I am going to fix the compliance issues for the profile template " + this.transform.hyperlink(templateHyperlink, templateName) + ".  Are you sure you want to do this? (@" + this.robot.name + " yes/@" + this.robot.name + " no)");
+    let nameAndHyperlink = getDeviceNameAndHyperLink(host + "/rest/server-profile-templates/" + msg.templateId);
+    this.transform.text(msg, msg.message.user.name + " I am going to fix the compliance issues for the profile template " + this.transform.hyperlink(nameAndHyperlink.hyperlink, nameAndHyperlink.deviceName) + ".  Are you sure you want to do this? (@" + this.robot.name + " yes/@" + this.robot.name + " no)");


Hmm...good question. Not sure what to do about this now. We may have to think about changing lots of our messaging now.

danielreed · 2017-08-02T02:07:32Z

src/listener/utils/transforms/server-hardware.js

@@ -54,7 +54,7 @@ export default class ServerHardware extends Resource {
      fields.push({
        title: 'Profile',
        short: true,
-        value: '<' + this.serverProfileHyperlink + '|' + getDeviceNameAndHyperLink(this.serverProfileUri).deviceName + '>'
+        value: '<' + this.serverProfileHyperlink + '|' + getDeviceNameAndHyperLink(host + '' + this.serverProfileUri).deviceName + '>'


Good catch I probably don't need to concatenate the strings this way.

danielreed · 2017-08-02T02:17:58Z

src/oneview-sdk/notifications.js

-        queue.subscribe({ack: true}, ::emitter.onMessage);
-      });
-    }
+    this.robot.logger.info('Connected to SCMB, waiting for messages');


danielreed · 2017-08-02T02:19:35Z

src/oneview.js

-    robot.logger.info('Logged into OV appliance.');
+
+  client.login(false).then(() => {
+    robot.logger.info('Logged into OV appliance(s).');


That does sound better.

danielreed · 2017-08-02T02:20:11Z

src/ov-brain.js

-          resourcesMap.set(sh.uri, {name: sh.name, hyperlink: sh.hyperlink, model: sh.model});
-          robot.logger.info('Found server hardware: (Name: ' + sh.name + ', Serial Number: ' + sh.serialNumber + ', URI: ' + sh.uri + ')');
+        for (let member of res.members) {
+          let uri = url.parse(member.hyperlink).hostname + '' + member.uri;


danielreed · 2017-08-02T02:20:29Z

src/ov-brain.js

-          resourcesMap.set(spt.uri, {name: spt.name, hyperlink: spt.hyperlink, model: undefined});
-          robot.logger.info('Found server profile template: (Name: ' + spt.name + ', URI: ' + spt.uri + ')');
+        for (let member of res.members) {
+          let uri = url.parse(member.hyperlink).hostname + '' + member.uri;


danielreed · 2017-08-02T02:21:31Z

src/ov-brain.js

+        let auth = client.getAuthToken(host);
+
+        let model = message.resource.model;
+        let uri = host + '' + message.resource.uri;


jesseolsen

Looks good. Thanks!

jesseolsen approved these changes Aug 1, 2017

View reviewed changes

danielreed commented Aug 2, 2017

View reviewed changes

Fixing minor issues.

384f332

jesseolsen approved these changes Aug 2, 2017

View reviewed changes

jesseolsen merged commit 2e752c7 into master Aug 2, 2017

jesseolsen deleted the multi branch August 2, 2017 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to support multiple appliances #114

Changes to support multiple appliances #114

danielreed commented Aug 1, 2017

jesseolsen left a comment

jesseolsen Aug 1, 2017

danielreed Aug 2, 2017

jesseolsen Aug 1, 2017

danielreed Aug 2, 2017

jesseolsen Aug 1, 2017

danielreed Aug 2, 2017

jesseolsen Aug 1, 2017

danielreed Aug 2, 2017

jesseolsen Aug 1, 2017

jesseolsen Aug 1, 2017

danielreed Aug 2, 2017

jesseolsen Aug 1, 2017

danielreed Aug 2, 2017

jesseolsen Aug 1, 2017

danielreed Aug 2, 2017

jesseolsen Aug 1, 2017

danielreed Aug 2, 2017

jesseolsen Aug 1, 2017

danielreed Aug 2, 2017

danielreed left a comment

danielreed Aug 2, 2017

danielreed Aug 2, 2017

danielreed Aug 2, 2017

danielreed Aug 2, 2017

danielreed Aug 2, 2017

danielreed Aug 2, 2017

danielreed Aug 2, 2017

danielreed Aug 2, 2017

danielreed Aug 2, 2017

danielreed Aug 2, 2017

jesseolsen left a comment

Changes to support multiple appliances #114

Changes to support multiple appliances #114

Conversation

danielreed commented Aug 1, 2017

jesseolsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielreed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesseolsen left a comment

Choose a reason for hiding this comment