Development: use `wrangler` locally (update NGINX/Dockerfile config) #10965

humitos · 2023-12-20T10:23:56Z

Use wrangler (development CF Worker) to run the CF worker locally inside our development environment. This allow us to run the exact same code locally than on production, allowing us to perform tests while making modifications on this script without deploying them.

With the current code, developers shouldn't notice anything different and they don't have to do anything. Addons should just work on projects that have Addons enabled the next time they call inv docker.up locally.

Some decisions that I made:

copy the js file from -ops here to be able to load the script with wrangler
add two new NGINX location that proxy to GitHub: this allows us to run the .js file from -ops without any modification
use the pre-compiled version of readthedocs-addons.js script to avoid developers to setup all the js environment locally
allow to test "local development changes in addons" by simply modifying a URL in the CF Worker file
pin versions of wrangler and nodejs for the new Docker container

Closes readthedocs/addons#217
Closes https://github.com/readthedocs/readthedocs-ops/issues/1435
Requires readthedocs/common#192

Related readthedocs/addons#217

humitos · 2024-02-06T12:53:30Z

dockerfiles/force-readthedocs-addons.js

+const addonsJs =
+  '<script async type="text/javascript" src="/_/static/javascript/readthedocs-addons.js"></script>';


If we want to test an addon we are developing, we can just change this URL to http://localhost:8000/readthedocs-addons.js while having npm run dev running and refresh the page (there is no need to restart Docker or anything) -- 💯

Note, that there is a inv docker.up -w flag that enables webpack hosting. This would be a good pattern to follow eventually. Making this a manual operation means it won't be repeatable or easy to use.

The development workflow with Webpack dev server hosting these assets is really good and should probably be the default for core team.

Yes. I'm happy to create an issue to track this work. I don't think we have a pattern for "given an environment variable make a decision on an asset file". We will need to change the content of this file in particular, so, it may require some research.

readthedocs/common#199

humitos · 2024-02-06T12:55:36Z

dockerfiles/nginx/proxito.conf.template

+    # Proxy the "readthedocs-addons.js" to be downloaded from GitHub
+    location /_/static/javascript/readthedocs-addons.js {
+        proxy_pass https://raw.githubusercontent.com/readthedocs/addons/$NGINX_ADDONS_GITHUB_TAG/dist/readthedocs-addons.js;
+        add_header Content-Type	"text/javascript; charset=utf-8" always;
+    }
+
+    location /_/static/javascript/readthedocs-addons.js.map {
+        proxy_pass https://raw.githubusercontent.com/readthedocs/addons/$NGINX_ADDONS_GITHUB_TAG/dist/readthedocs-addons.js.map;
+        add_header Content-Type	"text/javascript; charset=utf-8" always;
+    }


This allows us to use the exact same version of the CF Worker script in -ops than in .org repositories. Then, each time we deploy a new version of the addons, we can update NGINX_ADDONS_GITHUB_TAG to point to the newer tag 🚀

Having a rel branch or similar would be useful here, but probably not worth doing in this PR>

Makes sense. I need to think a little more about this. We are not following the same deploy pattern that with the other repositories, and we are always just doing the deploy from a tag.

I don't know exactly why, but I suppose it was to simplify things when I built the deployment (maybe I copied it from EthicalAds client? 🤔 ). However, it may worth to use the standardize it.

Being explicit here about the version I think is an improvement I feel.

I will say that overall I prefer managing our libraries with standard packaging/versioning, and would love to eventually replace the rel/relcorp pattern with standard dependency management (npm install @readthedocs/addons@1.3.3) as I think it ends up being confusing coordinating rel/relcorp across all our repos. At very least, using versioning for smaller/newer libraries feels like a step forward.

OK. So, for now, I will keep it as is and we can discuss further if we need to adapt this code in the future.

ericholscher

This is a pretty clean solution 👍

ericholscher · 2024-02-06T21:48:23Z

dockerfiles/nginx/proxito.conf.template

+    # Proxy the "readthedocs-addons.js" to be downloaded from GitHub
+    location /_/static/javascript/readthedocs-addons.js {
+        proxy_pass https://raw.githubusercontent.com/readthedocs/addons/$NGINX_ADDONS_GITHUB_TAG/dist/readthedocs-addons.js;
+        add_header Content-Type	"text/javascript; charset=utf-8" always;
+    }
+
+    location /_/static/javascript/readthedocs-addons.js.map {
+        proxy_pass https://raw.githubusercontent.com/readthedocs/addons/$NGINX_ADDONS_GITHUB_TAG/dist/readthedocs-addons.js.map;
+        add_header Content-Type	"text/javascript; charset=utf-8" always;
+    }


Having a rel branch or similar would be useful here, but probably not worth doing in this PR>

ericholscher · 2024-02-06T21:49:22Z

dockerfiles/nginx/wrangler.conf.template

+
+    # Wrangler serving
+    location / {
+        proxy_pass http://wrangler:8000;


Would be good to document how this works somewhere.. perhaps in a README as well as update the dev environment docs?

Good point 👍🏼 . I will take a look at our docs and see if it's a good fit there first.

I added a readme file in 1dd4c55. It's not perfect, but I wasn't sure what else write down there. I hope it helps 😄

I also updated our dev installation guide in 1830c02

agjohnson

This looks good, I still have some questions on how exactly Wrangler operates in front of Proxito, that would be good to clarify more. I'm excited to have this in place though, I've struggled to do QA on addons from the local application environment.

agjohnson · 2024-02-08T18:22:04Z

dockerfiles/force-readthedocs-addons.js

+const addonsJs =
+  '<script async type="text/javascript" src="/_/static/javascript/readthedocs-addons.js"></script>';


Note, that there is a inv docker.up -w flag that enables webpack hosting. This would be a good pattern to follow eventually. Making this a manual operation means it won't be repeatable or easy to use.

The development workflow with Webpack dev server hosting these assets is really good and should probably be the default for core team.

agjohnson · 2024-02-08T18:29:32Z

dockerfiles/nginx/proxito.conf.template

@@ -1,7 +1,7 @@
 # Proxito
 server {
-    listen 80 default_server;
-    server_name $NGINX_PROXITO_SERVER_NAME;
+    listen 8080;


Regarding the readme change below, it's not clear to me why proxito is moving to 8080 here. Is wrangler proxying to port 8080 now?

We are moving El Proxito away from port 80. This is because Wrangler is the one listening to 80 now, since it's the first application handling the request at *.devthedocs.org URLs (documentation URLs).

Once the request is received by Wrangler, it executes our .js script that performs another request behind the scenes (using fetch) to El Proxito (NGINX container in port 8080¹) and gets the "original response". Then, it modifies that response to inject the HTML tags required.

This is exactly how CF works in production.

Does this make sense?

Footnotes

8080 is just a random/standard port. ↩

Well I understood the topology of wrangler mimicking Cloudflare in front of all requests, but it's still not clear what exactly is configured to forward to port 8080.

Is this wrangler configuration somewhere? Or are you describing something special in our code to forward to this container/port?

readthedocs/common#192 might make it more clear?

Ah, yeah, this PR does not show how the wrangler command is executed. The PR that Eric pointed out is the one to take a look and I think it will make it more clear 👍🏼 . Let me know if this is still confusing.

humitos · 2024-02-12T16:08:07Z

I still have some questions on how exactly Wrangler operates in front of Proxito, that would be good to clarify more

Do you have specific questions here where I can help?

humitos · 2024-02-13T10:55:06Z

I will merge this PR so we can move forward with this and other folks can try it as well. I'm happy to add any other feedback you may have in a following PR.

Development: use wrangler locally (update NGINX/Dockerfile config)

16ae1ab

Related readthedocs/addons#217

humitos force-pushed the humitos/wrangler-locally branch from dcedc4b to 16ae1ab Compare February 6, 2024 12:13

humitos added 3 commits February 6, 2024 13:15

Add CF Worker .js script

802c76c

Proxy JS files to Addons GitHub's repository

1cac3ad

Point common to wrangler branch

44f0837

humitos commented Feb 6, 2024

View reviewed changes

humitos marked this pull request as ready for review February 6, 2024 13:02

humitos requested review from a team as code owners February 6, 2024 13:02

humitos requested review from agjohnson and stsewd February 6, 2024 13:02

auto-assign bot assigned humitos Feb 6, 2024

ericholscher reviewed Feb 6, 2024

View reviewed changes

agjohnson approved these changes Feb 8, 2024

View reviewed changes

humitos added 3 commits February 13, 2024 10:53

Docs: update development install page

1830c02

Add NGINX cache for addons js files

f9af782

Add small readme explaining how each NGINX configuration works

1dd4c55

humitos requested a review from a team as a code owner February 13, 2024 10:53

humitos mentioned this pull request Feb 13, 2024

wrangler: ability to serve addons from local development readthedocs/common#199

Open

update common/ to point to the latest version

70f7059

humitos merged commit ea43371 into main Feb 13, 2024
2 of 4 checks passed

humitos deleted the humitos/wrangler-locally branch February 13, 2024 11:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Development: use `wrangler` locally (update NGINX/Dockerfile config) #10965

Development: use `wrangler` locally (update NGINX/Dockerfile config) #10965

humitos commented Dec 20, 2023 •

edited

Loading

humitos Feb 6, 2024

agjohnson Feb 8, 2024

humitos Feb 12, 2024

humitos Feb 13, 2024

humitos Feb 6, 2024

ericholscher Feb 6, 2024

humitos Feb 7, 2024

agjohnson Feb 8, 2024 •

edited

Loading

humitos Feb 12, 2024

ericholscher left a comment

ericholscher Feb 6, 2024

ericholscher Feb 6, 2024

humitos Feb 7, 2024

humitos Feb 13, 2024

humitos Feb 13, 2024

agjohnson left a comment

agjohnson Feb 8, 2024

agjohnson Feb 8, 2024

humitos Feb 12, 2024

agjohnson Feb 12, 2024

ericholscher Feb 12, 2024

humitos Feb 13, 2024

humitos commented Feb 12, 2024

humitos commented Feb 13, 2024

		const addonsJs =
		'<script async type="text/javascript" src="/_/static/javascript/readthedocs-addons.js"></script>';

Development: use wrangler locally (update NGINX/Dockerfile config) #10965

Development: use wrangler locally (update NGINX/Dockerfile config) #10965

Conversation

humitos commented Dec 20, 2023 • 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

agjohnson Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericholscher 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

agjohnson 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

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Feb 12, 2024

humitos commented Feb 13, 2024

Development: use `wrangler` locally (update NGINX/Dockerfile config) #10965

Development: use `wrangler` locally (update NGINX/Dockerfile config) #10965

humitos commented Dec 20, 2023 •

edited

Loading

agjohnson Feb 8, 2024 •

edited

Loading