[RFR][SVCS-336]MFR issue: cannot render (videos, PDFs, or PDBs) locally when dockerized #259

TomBaxter · 2017-05-19T16:03:17Z

Purpose:

In local development, those MFR extensions that create javascript links to WB files, must use localhost instead of 192.168.168.167. This is because the request will come from the browser instead of the MFR docker instance.

Add catch for rendering in local development

Changes:

Add EXTENSION settings for LOCAL_HOST and DOCKER_LOCAL_HOST
Add mfr.extension.utils.get_local_export_url to ...
1. Switch DOCKER_LOCAL_HOST to LOCAL_HOST
2. Remove "mode" parameter
For renderers pdb, pdf, and video:
Check for DOCKER_LOCAL_HOST and call mfr.extension.utils.get_local_export_url

Side effects

None

Important

There is a related PR That adds EXTENSION_CONFIG_LOCAL_DEVELOPMENT=1 to .docker-compose.mfr.env . Would be nice if the were merged together, though nothing bad would happen if they weren't.

[#SVCS-336]

Create PR for OSF.io add this line to OSF.io .docker-compose.mfr.env EXTENSION_CONFIG_LOCAL_DEVELOPMENT=1

abought · 2017-05-19T16:04:43Z

mfr/extensions/video/render.py

@@ -13,7 +14,14 @@ class VideoRenderer(extension.BaseRenderer):
        ]).get_template('viewer.mako')

    def render(self):
-        return self.TEMPLATE.render(base=self.assets_url, url=self.url)
+        self_url = urlparse(self.url)


This problem could more generally also affect other renderers that directly serve the file content, such as PDB files. Might be worth checking for similar edge case bugs?

Thanks. Yep, already looking at updating pdf renderer.

Wait, so it also affects a third filetype? Good find. Hope we can DRY it a bit.

cslzchen

works as expected with my local env
looks good except a few style improvements

I also have a general question. Why this only fails for the three types but not others. ~~Is it worth it to make this generalized for all instead of by specific extension?~~

Back to Add'l Dev 🎆

cslzchen · 2017-09-26T15:10:58Z

mfr/extensions/video/render.py

@@ -13,7 +15,11 @@ class VideoRenderer(extension.BaseRenderer):
        ]).get_template('viewer.mako')

    def render(self):
-        return self.TEMPLATE.render(base=self.assets_url, url=self.url)
+        self_url = urlparse(self.metadata.download_url)
+        # Catch for local developement.


Typo: "development" (3 occurrences)

cslzchen · 2017-09-26T15:13:01Z

mfr/extensions/settings.py

@@ -0,0 +1,6 @@
+from mfr import settings
+
+config = settings.child('EXTENTION_CONFIG')


Typo: 'EXTENSTION_CONFIG' (1 occurence)

cslzchen · 2017-09-26T15:16:51Z

mfr/extensions/utils.py

+
+def development_export_url(urlparsed_url):
+        query_dict = parse_qs(urlparsed_url.query, keep_blank_values=True)
+        query_dict.pop('mode', None)


Why do we pop mode?

It's been quite a while since I worked on this and don't recall off the top of my head. In brief testing I was not able to come up with a good answer other then that it isn't needed. I can remove the pop, but it's very possible I had a good reason that I no longer recall.

This only affects local development. We can keep it there with the TODO note for you to recall the reason. :D

cslzchen · 2017-09-26T16:14:45Z

mfr/extensions/utils.py

+        urlparsed_url = urlparsed_url._replace(query=urlencode(query_dict, doseq=True))
+        urlparsed_url = urlparsed_url._replace(netloc="{}:{}".format(settings.LOCAL_HOST,
+                                                                     urlparsed_url.port))
+        return urlparsed_url


Style suggestions:

indent by 4

2 empty lines after imports

naming

docs/comments

Here is my example:

from urllib.parse import parse_qs, urlencode from mfr.extensions import settings def get_local_export_url(parsed_url): """ Replace the docker domain with localhost (local development only). :param parsed_url: the default export URL which is parsed by `urllib.parse.urlparse` :return: the tweaked export URL for local development """ query_dict = parse_qs(parsed_url.query, keep_blank_values=True) # TODO: why do we pop `mode`? query_dict.pop('mode', None) parsed_url = parsed_url._replace(query=urlencode(query_dict, doseq=True)) parsed_url = parsed_url._replace( netloc="{}:{}".format(settings.LOCAL_HOST, parsed_url.port) ) return parsed_url

cslzchen · 2017-09-26T16:24:44Z

mfr/extensions/pdb/render.py

@@ -16,9 +19,14 @@ class PdbRenderer(extension.BaseRenderer):
        ]).get_template('viewer.mako')

    def render(self):
+        self_url = urlparse(self.metadata.download_url)
+        # Catch for local developement.
+        if self_url.hostname == ext_settings.DOCKER_LOCAL_HOST:


Can we make this equation check a flag instead? For example, in mfr/extenstion/settings:

# only set to `true` for local development DOCKER_LOCAL_HOST = config.get('DOCKER_LOCAL_HOST', false)

And we check:

if ext_settings.DOCKER_LOCAL_HOST:

I see what you mean. However, I like the explicit DOCKER_LOCAL_HOST setting. I think it's less confusing. When someone sees the 192.168.168.167 address, they know just what we're talking about. Also, I like the explicitness of specifying the IP we will change, and no other. I'll split the difference with you and add another config LOCAL_DEVELOPMENT.

Agreed. What you have looks good! 👍
And probably no need for the flag I suggested. It leads to confusion and developers need to update local configuration.

DOH! too late already did it, though I can undo it if you don't like it. You will now need to either add LOCAL_DEVELOPMENT=1 to your ~.cos file for MFR or...
add this line to your OSF.io .docker-compose.mfr.env...
EXTENSION_CONFIG_LOCAL_DEVELOPMENT=1
I'll make a coresponding OSF.io PR when this PR is finalized.

I tried not to disturb the PR too much. It is better to have both in place (and it is) to make sure this only works for local docker environment.

cslzchen · 2017-09-26T16:26:59Z

mfr/extensions/pdf/render.py

-        return self.TEMPLATE.render(base=self.assets_url, url=self.metadata.download_url)
+        self_url = urlparse(self.metadata.download_url)
+        # Catch for local developement.
+        if self_url.hostname == settings.DOCKER_LOCAL_HOST:


Same as mentioned above.

TomBaxter · 2017-09-26T20:30:12Z

RE: general question
I updated the PR Purpose: and code doc with...

In local development, those MFR extensions that create links to WB files in TEMPLATE, must use localhost instead of 192.168.168.167. This is because the request will come from the browser instead of the MFR docker instance.

cslzchen

Looks good 🔥 . Move to PCR (Passed Code Review).
@felliott Please double check before moving to RTM.
@TomBaxter Please add a dependency for the OSF ticket you mentioned.

TomBaxter · 2017-09-27T13:55:51Z

related PR created.

felliott

Some refactorings needed, and a few questions about the implementation of the url munger.

felliott · 2017-11-08T15:17:51Z

mfr/extensions/pdb/render.py

@@ -16,9 +19,14 @@ class PdbRenderer(extension.BaseRenderer):
        ]).get_template('viewer.mako')

    def render(self):
+        self_url = urlparse(self.metadata.download_url)


The code here and in the render methods for the pdf and video exporters are almost identical. Could you move this into a decorator/utility method/base class/whatever to avoid the code duplication?

felliott · 2017-11-08T15:32:41Z

mfr/extensions/utils.py

+def get_local_export_url(urlparsed_url):
+    """
+    Replace the docker domain with localhost (for use in local development).
+


Minor: show a simple example, like:

http://localhost:7777/foo/bar => http://192.168.168.167:7777/foo/bar

felliott · 2017-11-08T15:33:23Z

mfr/extensions/utils.py

+    """
+
+    query_dict = parse_qs(urlparsed_url.query, keep_blank_values=True)
+    # TODO: why do we pop `mode`?


??? Did this come from somewhere else?

That was a question from @cslzchen that I couldn't remember the answer to.

felliott · 2017-11-08T15:35:34Z

mfr/extensions/utils.py

+    """
+    Replace the docker domain with localhost (for use in local development).
+
+    :param parsed_url: the default export URL which is parsed by `urllib.parse.urlparse`


Your docstring is out-of-date.

Completed, I think?

felliott · 2017-11-08T15:36:45Z

mfr/extensions/utils.py

+    urlparsed_url = urlparsed_url._replace(query=urlencode(query_dict, doseq=True))
+    urlparsed_url = urlparsed_url._replace(
+        netloc="{}:{}".format(settings.LOCAL_HOST, urlparsed_url.port)
+    )


I'm not familiar with the _replace method. What does it do, and why are you using a private method? Is there another way to do this?

I'm sure this can be done many other ways, but I was trying to do the least amount of converting. This way I am just manipulating the ParseResult, a namedtuple subclass. Dropping the "mode" query parameter and switching the netloc.

https://docs.python.org/3.6/library/collections.html#collections.somenamedtuple._replace

coveralls · 2017-11-13T22:19:23Z

Coverage increased (+0.2%) to 66.567% when pulling 0fd79a7 on TomBaxter:feature/svcs-336 into dd5dd3e on CenterForOpenScience:develop.

coveralls · 2017-11-14T14:47:10Z

Coverage increased (+0.2%) to 66.567% when pulling 7ca847c on TomBaxter:feature/svcs-336 into 9e9fc26 on CenterForOpenScience:develop.

felliott

Very close, almost all nitpicks. Two minor issues:

The name of the decorator should be more descriptive.
The popping of the mode parameter should either be explained or removed. Please try it without and see what breaks.

Cheers,
@felliott

felliott · 2017-11-17T20:04:29Z

mfr/extensions/pdb/render.py

@@ -5,6 +5,7 @@
 from mako.lookup import TemplateLookup

 from mfr.core import extension
+from mfr.extensions.utils import download_from_template


Nitpick: follow PEP8 import order, this should go after mfr.extensions.pdb

felliott · 2017-11-17T20:05:48Z

mfr/extensions/utils.py

@@ -0,0 +1,22 @@
+from urllib.parse import urlparse, parse_qs, urlencode
+from mfr.extensions import settings


nitpick: leave one space between import stanzas

felliott · 2017-11-17T20:06:51Z

mfr/extensions/settings.py

+# TEMPLATE to render, when using local docker development environment. Call will be coming from
+# browser instead of docker container.
+
+LOCAL_DEVELOPMENT = config.get_bool('LOCAL_DEVELOPMENT', 0)


Nitpick: 0 ==> False

I can make this change and it will work fine. The reason I picked 0 instead is because the corresponding osf.io .docker-compose.mfr.env file can't set LOCAL_DEVELOPMENT to True, must be a string, therefor I chose "1" as that is covered by setting.get_bool. You still want I should change it to False?

Everything else looks good, so I'll take care of it. But generally the settings should follow the idiom they're defined in. This is a python config file, so True and False are appropriate. The .docker-compose.*.env files are env files, so 1 and 0 are appropriate for them.

felliott · 2017-11-17T20:11:12Z

mfr/extensions/utils.py

+from mfr.extensions import settings
+
+
+def download_from_template(func):


The decorator looks good to me, but I'd like to bikeshed the name, download_from_template is a little nonspecific. Maybe munge_url_for_localdev or something like that? Or even munge_download_url if this might end up doing more than just rewriting the localdev download url. What do you think?

Sounds good. Completed.

felliott · 2017-11-17T20:12:57Z

mfr/extensions/utils.py

+        download_url = urlparse(self.metadata.download_url)
+        if (settings.LOCAL_DEVELOPMENT and download_url.hostname == settings.DOCKER_LOCAL_HOST):
+            query_dict = parse_qs(download_url.query, keep_blank_values=True)
+            query_dict.pop('mode', None)


Please remove this and see what breaks. If it's needed, add it back with a comment explaining why. If not, leave it out. If we can't figure out why we need it, we should leave it out until we know why.

Wasn't able to reproduce an issue, so removing. Completed.

felliott · 2017-11-17T20:14:53Z

mfr/extensions/utils.py

+            query_dict = parse_qs(download_url.query, keep_blank_values=True)
+            query_dict.pop('mode', None)
+            download_url = download_url._replace(query=urlencode(query_dict, doseq=True))
+            download_url = download_url._replace(


First, my bad: I thought the leading underscore meant this was a private method. Clearly I need to learn more python.

Nitpick: you can modify both fields of the named tuple in a single _replace command.

felliott · 2017-11-17T20:15:15Z

mfr/extensions/utils.py

+            query_dict.pop('mode', None)
+            download_url = download_url._replace(query=urlencode(query_dict, doseq=True))
+            download_url = download_url._replace(
+                netloc="{}:{}".format(settings.LOCAL_HOST, download_url.port)


Super-nitpick: please use single quotes for literals.

felliott · 2017-11-17T20:15:31Z

mfr/extensions/video/render.py

@@ -2,6 +2,7 @@

 from mako.lookup import TemplateLookup

+from mfr.extensions.utils import download_from_template
 from mfr.core import extension


nitpick: import order

felliott · 2017-11-17T20:20:51Z

mfr/extensions/utils.py

+def download_from_template(func):
+    """
+    Check if LOCAL_DEVELOPMENT and if so
+    Replace the docker domain with localhost (for use in local development).


Copyedit: "If MFR is being run in a local development environment (i.e. LOCAL_DEVELOPMENT is True), we need to replace the external host (the one the user provides, default: "localhost") with the internal host (the one the backend services communicate on, default: 192.168.168.167)."

Completed. Note: I had to switch the to/from. I probably confused you with the bad example.

TomBaxter

CRR completed.

TomBaxter · 2017-11-20T16:58:42Z

mfr/extensions/settings.py

+# TEMPLATE to render, when using local docker development environment. Call will be coming from
+# browser instead of docker container.
+
+LOCAL_DEVELOPMENT = config.get_bool('LOCAL_DEVELOPMENT', 0)


I can make this change and it will work fine. The reason I picked 0 instead is because the corresponding osf.io .docker-compose.mfr.env file can't set LOCAL_DEVELOPMENT to True, must be a string, therefor I chose "1" as that is covered by setting.get_bool. You still want I should change it to False?

TomBaxter · 2017-11-20T16:59:19Z

mfr/extensions/pdb/render.py

@@ -5,6 +5,7 @@
 from mako.lookup import TemplateLookup

 from mfr.core import extension
+from mfr.extensions.utils import download_from_template


TomBaxter · 2017-11-20T17:19:06Z

mfr/extensions/utils.py

@@ -0,0 +1,22 @@
+from urllib.parse import urlparse, parse_qs, urlencode
+from mfr.extensions import settings


TomBaxter · 2017-11-20T17:23:10Z

mfr/extensions/utils.py

+from mfr.extensions import settings
+
+
+def download_from_template(func):


Sounds good. Completed.

TomBaxter · 2017-11-20T18:37:13Z

mfr/extensions/utils.py

+def download_from_template(func):
+    """
+    Check if LOCAL_DEVELOPMENT and if so
+    Replace the docker domain with localhost (for use in local development).


Completed. Note: I had to switch the to/from. I probably confused you with the bad example.

TomBaxter · 2017-11-20T18:44:34Z

mfr/extensions/utils.py

+            query_dict = parse_qs(download_url.query, keep_blank_values=True)
+            query_dict.pop('mode', None)
+            download_url = download_url._replace(query=urlencode(query_dict, doseq=True))
+            download_url = download_url._replace(


TomBaxter · 2017-11-20T18:44:42Z

mfr/extensions/utils.py

+            query_dict.pop('mode', None)
+            download_url = download_url._replace(query=urlencode(query_dict, doseq=True))
+            download_url = download_url._replace(
+                netloc="{}:{}".format(settings.LOCAL_HOST, download_url.port)


TomBaxter · 2017-11-20T18:45:39Z

mfr/extensions/video/render.py

@@ -2,6 +2,7 @@

 from mako.lookup import TemplateLookup

+from mfr.extensions.utils import download_from_template
 from mfr.core import extension


TomBaxter · 2017-11-20T19:07:14Z

mfr/extensions/utils.py

+        download_url = urlparse(self.metadata.download_url)
+        if (settings.LOCAL_DEVELOPMENT and download_url.hostname == settings.DOCKER_LOCAL_HOST):
+            query_dict = parse_qs(download_url.query, keep_blank_values=True)
+            query_dict.pop('mode', None)


Wasn't able to reproduce an issue, so removing. Completed.

Add EXTENSION settings for.. LOCAL_HOST, DOCKER_LOCAL_HOST, and LOCAL_DEVELOPMENT Add mfr.extension.utils.download_from_template decorator to... 1. Switch DOCKER_LOCAL_HOST to LOCAL_HOST 2. Remove "mode" parameter For renderers pdb, pdf, and video: Add download_from_template decorator to render method Fix test for video renderer In local development, those MFR extensions that create links to WB files in TEMPLATES, must use localhost instead of 192.168.168.167. This is because the request will come from the browser instead of the MFR docker instance.

[#SVCS-336]

coveralls · 2017-11-20T19:58:20Z

Coverage increased (+0.2%) to 68.242% when pulling d7b598d on TomBaxter:feature/svcs-336 into 8bb2dd4 on CenterForOpenScience:develop.

felliott · 2017-11-20T21:31:42Z

A!W!E!S!O!M!E! Moving to RTM.

[SVCS-336] Closes: #259

abought reviewed May 19, 2017

View reviewed changes

TomBaxter changed the title ~~[RFR][SVCS-336]MFR issue: cannot render videos locally when dockerized~~ [RFR][SVCS-336]MFR issue: cannot render (videos, PDFs, or PDBs) locally when dockerized May 19, 2017

cslzchen reviewed Sep 26, 2017

View reviewed changes

cslzchen approved these changes Sep 26, 2017

View reviewed changes

cslzchen added the Final Review label Oct 12, 2017

felliott requested changes Nov 8, 2017

View reviewed changes

felliott requested changes Nov 17, 2017

View reviewed changes

TomBaxter mentioned this pull request Nov 20, 2017

[RFR] [SVCS-336] Add env variable required for some file types to render in docker CenterForOpenScience/osf.io#7740

Merged

TomBaxter commented Nov 20, 2017

View reviewed changes

TomBaxter added 2 commits November 20, 2017 14:11

CRR style, method naming, docs string

d7b598d

[#SVCS-336]

felliott added Ready To Merge and removed Final Review labels Nov 20, 2017

felliott added a commit that referenced this pull request Nov 20, 2017

Merge branch 'feature/docker-local-urls' into next

c6f0d2c

[SVCS-336] Closes: #259

felliott closed this in ec620af Jan 23, 2018

		@@ -0,0 +1,6 @@
		from mfr import settings

		config = settings.child('EXTENTION_CONFIG')

		@@ -0,0 +1,22 @@
		from urllib.parse import urlparse, parse_qs, urlencode
		from mfr.extensions import settings

		from mfr.extensions import settings


		def download_from_template(func):

[RFR][SVCS-336]MFR issue: cannot render (videos, PDFs, or PDBs) locally when dockerized #259

[RFR][SVCS-336]MFR issue: cannot render (videos, PDFs, or PDBs) locally when dockerized #259

Conversation

TomBaxter commented May 19, 2017 • edited

Purpose:

Changes:

Side effects

Important

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cslzchen left a comment • edited

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

TomBaxter commented Sep 26, 2017

cslzchen left a comment • edited

Choose a reason for hiding this comment

TomBaxter commented Sep 27, 2017

felliott 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

coveralls commented Nov 13, 2017 • edited

coveralls commented Nov 14, 2017 • edited

felliott 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

TomBaxter 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

coveralls commented Nov 20, 2017 • edited

TomBaxter commented May 19, 2017 •

edited

cslzchen left a comment •

edited

cslzchen left a comment •

edited

coveralls commented Nov 13, 2017 •

edited

coveralls commented Nov 14, 2017 •

edited

coveralls commented Nov 20, 2017 •

edited