Handle fallbackRegistryUrl for ~info and ~preview #380

pbazydlo · 2017-02-09T15:52:42Z

The aim of this PR is to fill in some of the missing pieces for fallbackRegistryUrl toggle.
The fallback functionality is added to ~info and ~preview endpoints.

It is probably best to read this PR in whitespace insensitive mode as some code is nested into one more callback (so it became more indented): https://github.com/opentable/oc/pull/380/files?w=1

Changes to component-info and component-preview are conceptually identical so I suggest reviewing from component-preview file as it is little bit smaller, so more manageable.

pbazydlo · 2017-02-09T16:58:44Z

src/registry/router.js

-    componentInfo: new ComponentInfoRoute(repository),
-    componentPreview: new ComponentPreviewRoute(repository),
+    componentInfo: new ComponentInfoRoute(conf, repository),
+    componentPreview: new ComponentPreviewRoute(conf, repository),


those two endpoints now need configuration for fallbackRegistryUrl

pbazydlo · 2017-02-09T17:00:08Z

src/registry/routes/component-info.js


-module.exports = function(repository){
-  return function(req, res){
+function getParams(component) {


I've split this function from the main callback, because it seemed little bit hard to read.

This kind of refactor probably shouldn't be in this PR, so if it is prepared to revert this change I can make a commit for that.

pbazydlo · 2017-02-09T17:00:18Z

src/registry/routes/component-info.js

-      if(err){
-        res.errorDetails = err;
-        return res.status(404).json({ err: err });
+function getParsedAuthor(component) {


I've split this function from the main callback, because it seemed little bit hard to read.

This kind of refactor probably shouldn't be in this PR, so if it is prepared to revert this change I can make a commit for that.

pbazydlo · 2017-02-09T17:00:25Z

src/registry/routes/component-info.js

+  return _.isString(author) ? parseAuthor(author) : author;
+}
+
+function addGetRepositoryUrlFunction(component) {


I've split this function from the main callback, because it seemed little bit hard to read.

This kind of refactor probably shouldn't be in this PR, so if it is prepared to revert this change I can make a commit for that.

pbazydlo · 2017-02-09T17:02:48Z

src/registry/routes/component-info.js

-        var params = {},
-            author = component.author || {},
-            parsedAuthor = _.isString(author) ? parseAuthor(author) : author;
+      getComponentInfoOrPreviewFallback(conf, req, res, localRegistryError, localComponent, function(err, component) {


this is the function that will try to do a fallback call to registry url

I've also looked briefly at replacing repository.getComponent with getcomponenthelper, however, helper returned very different format of response (that I would not be able to map).

pbazydlo · 2017-02-09T17:09:34Z

src/registry/routes/helpers/get-component-fallback.js

+      return callback(res[0]);
+    });
+  },
+  getComponentInfoOrPreviewFallback: function (conf, req, res, localRegistryError, component, callback) {


I had some trouble coming up with good names for this and the other (getComponent) functions.

The getComponent function goes to fallback repository and makes a POST query for a rendered component.

The getComponentInfoOrPreviewFallback function repeats a GET query to fallback registry and was tested only with ~info and ~preview endpoints.

I think I prefer if we keep it modularised to keep consistency, example get-component-info-fallback.js and get-component-fallback.js. Each thing exposes one thing with the same name.

pbazydlo · 2017-02-09T17:10:52Z

src/registry/routes/helpers/get-component-fallback.js

+    return request({
+      method: 'get',
+      url: conf.fallbackRegistryUrl + path.substr(1, path.length - 1),


req.originalUrl comes with a / as first character and we already have trailing / in conf.fallbackRegistryUrl is it prefered if I used url library or is this fine?

as said there ^

matteofigus

Also, can you review #382 first and then rebase this with that?

matteofigus · 2017-02-14T09:56:45Z

src/registry/routes/helpers/get-component.js

@@ -12,7 +12,7 @@ var Client = require('../../../../client');
 var detective = require('../../domain/plugins-detective');
 var eventsHandler = require('../../domain/events-handler');
 var GetComponentRetrievingInfo = require('./get-component-retrieving-info');
-var getComponentFallback = require('./get-component-fallback');
+var getComponentFallback = require('./get-component-fallback').getComponent;


This is a bit inconsistent. Can we just expose it as we do with all the other stuff?

matteofigus · 2017-02-14T10:00:26Z

src/registry/routes/helpers/get-component-fallback.js

+      return callback(res[0]);
+    });
+  },
+  getComponentInfoOrPreviewFallback: function (conf, req, res, localRegistryError, component, callback) {


I think I prefer if we keep it modularised to keep consistency, example get-component-info-fallback.js and get-component-fallback.js. Each thing exposes one thing with the same name.

matteofigus · 2017-02-14T10:04:07Z

src/registry/routes/helpers/get-component-fallback.js

I think I am not in love about using req.originalUrl. Can we build the url by accessing component variable and using domain/url-builder.js?

matteofigus · 2017-02-14T10:05:02Z

src/registry/routes/helpers/get-component-fallback.js

+    return request({
+      method: 'get',
+      url: conf.fallbackRegistryUrl + path.substr(1, path.length - 1),


as said there ^

matteofigus

All looks really good. Only thing, the naming conflict I mentioned in my comment.

matteofigus · 2017-03-09T20:24:17Z

src/registry/routes/component-info.js

+module.exports = function(conf, repository){
+  return function(req, res){
+    repository.getComponent(req.params.componentName, req.params.componentVersion, function(localRegistryError, localComponent){
+      if(localRegistryError && conf.fallbackRegistryUrl) {


One little thing here in regards of naming. In many other places in the code, we refer "local" registry as registry running as oc dev, as way to differentiate it from the "default" registry running as real instance.

I think what you are calling local here can be confused with the other "local" so I think I would prefer us to change the naming. What about making local => default (so registryError, component) and the other default => fallback (so fallbackError, fallbackComponent)?

Another idea is to call the "local" "current"? Or maybe you have other ideas?

Makes sense for me

matteofigus

lol found more local references. I think I highlighted them all this time 🎉

matteofigus · 2017-03-10T12:51:29Z

src/registry/routes/helpers/get-component-fallback.js

-      });
+
+    if (fallbackErr) {
+      return callback({localError: localRegistryError, fallbackError: fallbackErr});


another local reference

matteofigus · 2017-03-10T12:51:36Z

src/registry/routes/helpers/get-component-fallback.js

+    try {
+      return callback(null, JSON.parse(fallbackResponse));
+    } catch (parseError) {
+      return callback({localError: localRegistryError, fallbackError: 'Could not parse fallback response: ' + fallbackResponse});


local reference

matteofigus · 2017-03-10T12:51:48Z

src/registry/routes/helpers/get-component-fallback.js

+    });
+  },
+  getComponentPreview: function (conf, req, res, localRegistryError, callback) {


local reference

matteofigus · 2017-03-10T12:51:56Z

src/registry/routes/helpers/get-component-fallback.js

+  },
+  getComponentPreview: function (conf, req, res, localRegistryError, callback) {
+    getComponentFallbackForViewType(urlBuilder.componentPreview, conf, req, res, localRegistryError, callback);


local reference

matteofigus · 2017-03-10T12:52:02Z

src/registry/routes/helpers/get-component-fallback.js

+    getComponentFallbackForViewType(urlBuilder.componentPreview, conf, req, res, localRegistryError, callback);
+  },
+  getComponentInfo: function (conf, req, res, localRegistryError, callback) {


local reference

matteofigus · 2017-03-10T12:52:07Z

src/registry/routes/helpers/get-component-fallback.js

+  },
+  getComponentInfo: function (conf, req, res, localRegistryError, callback) {
+    getComponentFallbackForViewType(urlBuilder.componentInfo, conf, req, res, localRegistryError, callback);


local reference

matteofigus · 2017-03-10T12:52:23Z

src/registry/routes/component-info.js

-      var isHtmlRequest = !!req.headers.accept && req.headers.accept.indexOf('text/html') >= 0;
+function componentInfo(err, req, res, component) {
+  if(err) {
+    res.errorDetails = err.localError;


local reference

matteofigus · 2017-03-10T12:52:32Z

src/registry/routes/component-preview.js

-        res.errorDetails = err;
+function componentPreview(err, req, res, component) {
+    if(err) {
+        res.errorDetails = err.localError;


local reference

matteofigus · 2017-03-10T12:52:42Z

src/registry/routes/helpers/get-component-fallback.js

 var _ = require('underscore');

-module.exports = function(fallbackRegistryUrl, headers, component, callback) {
+function getComponentFallbackForViewType(buildUrl, conf, req, res, localRegistryError, callback) {


local reference

damn, I should have checked.

pbazydlo · 2017-03-10T14:01:32Z

@matteofigus this time should be good 💨

pbazydlo added the wip label Feb 9, 2017

pbazydlo requested a review from jankowiakmaria February 9, 2017 15:52

pbazydlo commented Feb 9, 2017

View reviewed changes

pbazydlo added cli enhancement and removed wip labels Feb 9, 2017

pbazydlo changed the title ~~[WIP] Handle fallbackRegistryUrl for ~info and ~preview~~ Handle fallbackRegistryUrl for ~info and ~preview Feb 9, 2017

pbazydlo requested a review from matteofigus February 9, 2017 17:34

matteofigus mentioned this pull request Feb 14, 2017

Consistency #382

Merged

matteofigus requested changes Feb 14, 2017

View reviewed changes

pbazydlo and others added 8 commits March 9, 2017 13:43

Initial fallback for info and preview

1f23561

Registry is hiding fallback registry behind itself

eeedd9d

Clean up component info code

67d90aa

Move all of fallback logic to get-component-fallback

a0cb5cd

Add tests for info and preview endpoint fallback

f46695f

Fallback naming cleanup

34f7b44

Cleanup info and preview fallback

dffbbce

Don't pick specific funciton in require

b2e61e6

pbazydlo force-pushed the improveFallbackRegistryForDev branch from b3afac9 to b2e61e6 Compare March 9, 2017 13:43

Fix tests after rebase

5f6b306

matteofigus requested changes Mar 9, 2017

View reviewed changes

matteofigus requested changes Mar 10, 2017

View reviewed changes

pbazydlo force-pushed the improveFallbackRegistryForDev branch from d9fed88 to c35ef82 Compare March 10, 2017 13:23

Change naming in fallbacks for more clarity

5041069

pbazydlo force-pushed the improveFallbackRegistryForDev branch from c35ef82 to 5041069 Compare March 10, 2017 13:27

matteofigus approved these changes Mar 10, 2017

View reviewed changes

matteofigus merged commit 36167f0 into master Mar 10, 2017

matteofigus deleted the improveFallbackRegistryForDev branch March 10, 2017 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle fallbackRegistryUrl for ~info and ~preview #380

Handle fallbackRegistryUrl for ~info and ~preview #380

pbazydlo commented Feb 9, 2017 •

edited

Loading

pbazydlo Feb 9, 2017

pbazydlo Feb 9, 2017

pbazydlo Feb 9, 2017

pbazydlo Feb 9, 2017

pbazydlo Feb 9, 2017

pbazydlo Feb 9, 2017

matteofigus Feb 14, 2017

pbazydlo Feb 9, 2017

matteofigus Feb 14, 2017

matteofigus left a comment

matteofigus Feb 14, 2017

matteofigus Feb 14, 2017

matteofigus Feb 14, 2017

matteofigus Feb 14, 2017

matteofigus left a comment

matteofigus Mar 9, 2017

jankowiakmaria Mar 10, 2017

matteofigus left a comment

matteofigus Mar 10, 2017

matteofigus Mar 10, 2017

matteofigus Mar 10, 2017

matteofigus Mar 10, 2017

matteofigus Mar 10, 2017

matteofigus Mar 10, 2017

matteofigus Mar 10, 2017

matteofigus Mar 10, 2017

matteofigus Mar 10, 2017

pbazydlo Mar 10, 2017

pbazydlo commented Mar 10, 2017

Handle fallbackRegistryUrl for ~info and ~preview #380

Handle fallbackRegistryUrl for ~info and ~preview #380

Conversation

pbazydlo commented Feb 9, 2017 • edited Loading

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

matteofigus 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

matteofigus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matteofigus 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

pbazydlo commented Mar 10, 2017

pbazydlo commented Feb 9, 2017 •

edited

Loading