Add feature flag and config setting to load client from an external URL #4355

robertknight · 2017-02-07T16:00:48Z

Add a feature flag (use_client_boot_script) and a configuration setting (h.client_url) which causes the service to use the client's boot script instead of using the boot code baked into the service itself. When the flag is enabled:

The /embed.js view returns a redirect to the client's entry point at https://unpkg.com/hypothesis or the custom location set with the h.client_url setting.
The app.html view returns a minimal HTML page which just includes the client URL.

Together, these changes will enable us to deploy and serve the client from a CDN rather than baking it into the Docker image for the service.

Manual testing:

Check out this branch
In the client repo, check out Add an express server which serves the package's contents. client#229.
In the client repo, set the H_SERVICE_URL env var to http://localhost:5000 and run gulp watch
In the h repo, set the the CLIENT_URL env var to http://localhost:3001/hypothesis and run make dev
Go to /docs/help and /stream and verify that the client loads. Also verify that the client's CSS and JS assets are loaded from http://localhost:3001.

robertknight · 2017-02-07T16:01:18Z

WIP because it needs more tests.

codecov-io · 2017-02-07T16:03:04Z

Codecov Report

Merging #4355 into master will decrease coverage by -0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4355      +/-   ##
==========================================
- Coverage   94.24%   94.23%   -0.02%     
==========================================
  Files         319      319              
  Lines       18017    18071      +54     
  Branches     1062     1065       +3     
==========================================
+ Hits        16980    17029      +49     
+ Misses        938      936       -2     
- Partials       99      106       +7

Impacted Files	Coverage Δ
h/models/feature.py	`93.1% <ø> (ø)`	✅
h/config.py	`64.86% <ø> (ø)`	✅
h/views/client.py	`84.84% <100%> (-4.29%)`	❌
tests/h/views/main_test.py	`97.97% <100%> (+0.08%)`	✅
tests/h/views/client_test.py	`100% <100%> (ø)`	✅
src/memex/search/init.py	`38.88% <ø> (ø)`	✅
h/stats.py	`83.33% <ø> (ø)`	✅
h/session.py	`91.83% <ø> (ø)`	✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f545b5...7acfabd. Read the comment docs.

robertknight · 2017-02-08T07:36:34Z

tests/h/views/client_test.py

+
+
+@pytest.fixture
+def pyramid_request_(pyramid_request):


This fixture needs to be named differently than the pyramid_request fixture which it extends, as otherwise the registry gets reset to a new object before individual test cases actually run.

nickstenning · 2017-02-08T14:39:11Z

I think it would make more sense to have the feature flag operate at the view level and actually request from upstream (and then serve with appropriate cache headers), rather than having embed.js be a bootstrapper for a bootstrapper.

robertknight · 2017-02-09T12:48:14Z

This is currently blocked on #4363 which simplifies the client views a lot and makes them easier to test.

robertknight · 2017-02-17T12:27:30Z

Slack discussion: https://hypothes-is.slack.com/archives/public/p1487328023004988

nickstenning

Generally looks good, and does the right thing, but a couple of points (in more detail inline):

splitting the /embed.js view into two (one for each value of the feature flag) will simplify the code and tests
I might be missing something, but I don't think we need to "resolve" the URL when using it in app.html
Setting defaults don't live in .ini files any more.

nickstenning · 2017-02-17T10:21:54Z

h/templates/embed.js.jinja2

+  configEl.type = 'application/json';
+  configEl.textContent = JSON.stringify({
+    assetRoot: '{{ client_asset_root }}',
+    sidebarAppUrl: '{{ app_html_url }}',


As I said before, putting this here still doesn't make a lot of sense to me, as it implies that the entrypoint isn't actually usable on its own, i.e. including https://unpkg.com/hypothesis in the page won't actually work.

Is there a reason we can't hard code the defaults in the client itself? They can be overrideable, of course, but having sensible defaults for the https://hypothes.is service in the client build would mean that we don't need to to this at all.

The defaults are now hardcoded to correct values in the client, with one caveat - the /app.html URL can only be hardcoded to one value - so if we remove this config it won't be possible to load the QA version of /app.html on the QA version of the service.

nickstenning · 2017-02-20T14:02:43Z

conf/app.ini

@@ -19,6 +19,7 @@ use: call:h.app:create_app
 # If not provided, both default to a random URL-safe base64-encoded string.
 #h.client_id:
 #h.client_secret:
+h.client_url: https://unpkg.com/hypothesis


We're slowly but surely phasing out use of these configuration files, so perhaps we could put the default value in an appropriate module in the application?

Ah, I didn't know we were phasing these out. I can do that.

nickstenning · 2017-02-20T14:03:13Z

h/templates/app.html.jinja2

@@ -9,7 +9,7 @@
     only ever displays a single route.
     #}
    <base target="_top" href="/" />
-  {% for url in app_css_urls %}
+    {% for url in app_css_urls %}


Doesn't seem like this diff is relevant to these changes, is it?

No, I think it stems from an indentation correction made during an earlier version of this PR.

nickstenning · 2017-02-20T14:20:44Z

h/views/client.py

-        'inject_resource_urls': (absolute_asset_urls('inject_js') +
-                                 absolute_asset_urls('inject_css'))
-    }
+    client_boot_url = _resolve_client_boot_url(request)


Rather than adding complexity to this view function, it probably makes more sense to have a separate view function which uses the has_feature_flag view predicate:

from pyramid.config import not_ ... @view_config(route_name='embed', renderer='h:templates/embed.js.jinja2', has_feature_flag=not_('use_client_boot_script')) def embed(request): # current view @view_config(route_name='embed', has_feature_flag='use_client_boot_script') def embed_redirect(request): # redirect

nickstenning · 2017-02-20T14:28:01Z

h/views/client.py

+
+    if client_boot_url:
+        rsp = HTTPFound(location=client_boot_url)
+        rsp.cache_control.max_age = 60 * 5  # 5 minutes


Let's use the http_cache arg to view_config instead, as that will ensure we set both Expires and C-C headers.

nickstenning · 2017-02-20T14:30:42Z

h/views/client.py

@@ -85,6 +112,7 @@ def sidebar_app(request, extra=None):
    ga_client_tracking_id = settings.get('ga_client_tracking_id')

    ctx = _app_html_context(api_url=request.route_url('api.index'),
+                            client_boot_url=_resolve_client_boot_url(request),


Is there a reason we need to actually resolve the URL here? Can we use the value of the setting without making any HTTP requests?

The client will have an /embed.js => https://unpkg.com/hypothesis@0.X.Y redirect cached locally but won't have an https://unpkg.com/hypothesis => https://unpkg.com/hypothesis@0.X.Y redirect cached locally because the service proxies the original redirect. So not resolving the URL would require the client to perform an extra request.

In an earlier version of this PR, I just had /app.html include /embed.js directly (which the client will have cached). Come to think of it, I think I could just revert back to doing that.

Add a feature flag which causes the service to use the client's boot script instead of using the boot logic and bundle list from the service. This is an initial step towards serving the client from an external CDN instead of from the Hypothesis service.

…t from Add a setting which specifies the URL for the client's boot script. Client assets are expected to be loaded from the same URL, but with a trailing slash added. The latest stable production client is loaded by default. It can be overridden in local development by setting the `CLIENT_URL` env var.

The client now includes the necessary config baked in, so the service does not need to provide it. For context, see Slack discussion https://hypothes-is.slack.com/archives/public/p1487328023004988

Split 'embed' view into two separate views with `has_feature_flag` view predicates. Since the view behaves completely differently depending on the value of the flag, this produces a simpler result.

As requested during CR, remove the `h.client_url` setting from the service's config INI files in favor of defining a default URL in `h.views.client` which is used unless the setting has been defined using the CLIENT_URL env var. This commit also removes the `assetRoot` configuration from the `/app.html` route. This config info is no longer needed or used by the client's boot script which now has the URL hardcoded in.

The `_client_url` helper already tests the feature flag and returns `None` if it is disabled.

Initialize it in the same way as `app_js_urls` for consistency.

nickstenning · 2017-02-22T13:27:42Z

h/views/client.py

-    app_css_urls = assets_env.urls('app_css')
-    app_js_urls = assets_env.urls('app_js')
+    if client_url:
+        app_js_urls = [client_url]


I thought you were going to use request.route_path('embed') here to avoid having to do an HTTP call every time we render app.html -- which is notably not served with sensible cache headers at the moment.

Ah, yes indeed. Done.

nickstenning · 2017-02-22T13:31:23Z

h/views/client.py

@@ -132,6 +176,20 @@ def absolute_asset_urls(bundle_name):
    }


+@view_config(route_name='embed',
+             has_feature_flag='use_client_boot_script',
+             http_cache=60 * 5)


We want this to be cached by the CDN, so need to specify the public C-C token: http_cache=(300, {'public': True}) as with robots.txt.

Hmm, although we should probably not do that while it's controlled by a feature flag. Leave this one for now and we'll follow up once we've deployed it.

Actually, no, it's fine. We send Vary: Cookie so C-C: public should be fine here.

Good catch. Done.

nickstenning · 2017-02-22T13:36:41Z

Two small changes needed:

to the Cache-Control headers for /embed.js so that it can be cached by the CDN and not just by user-agents.
to /app.html so that we don't put an external HTTP request on the critical path.

This saves an extra request as the client will already have /embed.js cached.

These functions are never called when the feature flag is off.

robertknight added the WIP label Feb 7, 2017

robertknight commented Feb 8, 2017

View reviewed changes

robertknight removed the WIP label Feb 8, 2017

robertknight force-pushed the use-client-boot-script branch from c5890a5 to 600a894 Compare February 8, 2017 07:44

robertknight mentioned this pull request Feb 8, 2017

Simplify embed.js view #4359

Closed

robertknight added the WIP label Feb 9, 2017

robertknight force-pushed the use-client-boot-script branch from 600a894 to 763abad Compare February 10, 2017 13:36

robertknight mentioned this pull request Feb 10, 2017

Add a new entry point script for the client application hypothesis/client#215

Merged

robertknight force-pushed the use-client-boot-script branch 2 times, most recently from 4ef4081 to b25880b Compare February 16, 2017 15:59

robertknight mentioned this pull request Feb 16, 2017

Add an express server which serves the package's contents. hypothesis/client#229

Merged

robertknight force-pushed the use-client-boot-script branch from b25880b to 2072f16 Compare February 16, 2017 16:14

robertknight removed the WIP label Feb 16, 2017

robertknight changed the title ~~Add feature flag to use client boot script~~ Add feature flag and config setting to load client from an external URL Feb 16, 2017

robertknight force-pushed the use-client-boot-script branch from 2072f16 to 47eec78 Compare February 17, 2017 17:31

nickstenning suggested changes Feb 20, 2017

View reviewed changes

robertknight added 7 commits February 21, 2017 11:33

Redirect to boot script rather than returning it

4bc24e3

The client now includes the necessary config baked in, so the service does not need to provide it. For context, see Slack discussion https://hypothes-is.slack.com/archives/public/p1487328023004988

Split 'embed' into two view functions

d4ece58

Split 'embed' view into two separate views with `has_feature_flag` view predicates. Since the view behaves completely differently depending on the value of the flag, this produces a simpler result.

Remove an unintentional indentation change

ffee7e6

Remove a couple of unused routes and a feature flag change

9240adc

robertknight force-pushed the use-client-boot-script branch from 47eec78 to 9240adc Compare February 21, 2017 14:20

Remove an unnecessary feature test

758eff6

The `_client_url` helper already tests the feature flag and returns `None` if it is disabled.

Move initialization of app_css_urls

f1147e9

Initialize it in the same way as `app_js_urls` for consistency.

nickstenning reviewed Feb 22, 2017

View reviewed changes

robertknight added 3 commits February 22, 2017 14:08

Load /embed.js rather than client URL directly in /app.html

ab5e472

This saves an extra request as the client will already have /embed.js cached.

Add 'public' modifier to Cache Control header for /embed.js

33a1f5d

Remove a couple of unused branches

7acfabd

These functions are never called when the feature flag is off.

nickstenning approved these changes Feb 23, 2017

View reviewed changes

nickstenning merged commit c30a649 into master Feb 23, 2017

nickstenning deleted the use-client-boot-script branch February 23, 2017 12:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feature flag and config setting to load client from an external URL #4355

Add feature flag and config setting to load client from an external URL #4355

robertknight commented Feb 7, 2017 •

edited

robertknight commented Feb 7, 2017

codecov-io commented Feb 7, 2017 •

edited

robertknight Feb 8, 2017

nickstenning commented Feb 8, 2017

robertknight commented Feb 9, 2017

robertknight commented Feb 17, 2017

nickstenning left a comment

nickstenning Feb 17, 2017

robertknight Feb 20, 2017

nickstenning Feb 20, 2017

robertknight Feb 20, 2017

nickstenning Feb 20, 2017

robertknight Feb 20, 2017

nickstenning Feb 20, 2017

nickstenning Feb 20, 2017

nickstenning Feb 20, 2017

robertknight Feb 20, 2017

nickstenning Feb 22, 2017

robertknight Feb 22, 2017

nickstenning Feb 22, 2017

nickstenning Feb 22, 2017

nickstenning Feb 22, 2017

robertknight Feb 22, 2017

nickstenning commented Feb 22, 2017

Add feature flag and config setting to load client from an external URL #4355

Add feature flag and config setting to load client from an external URL #4355

Conversation

robertknight commented Feb 7, 2017 • edited

robertknight commented Feb 7, 2017

codecov-io commented Feb 7, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

nickstenning commented Feb 8, 2017

robertknight commented Feb 9, 2017

robertknight commented Feb 17, 2017

nickstenning 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

nickstenning commented Feb 22, 2017

robertknight commented Feb 7, 2017 •

edited

codecov-io commented Feb 7, 2017 •

edited