Remap ports to the non-conflicting range #844

dhruvkb · 2022-08-03T09:59:22Z

Fixes

Addresses WordPress/openverse#200

Description

This PR

removes binding for ports that are only used for internal communication
changes port mappings to ports in the 502xx range to prevent any conflicts with services running on the host
updates documentation and tests to keep up with the changes
fixes some small side issues along the way
- removing unused settings
- using env vars in the run.sh entrypoint code and aligning it with the justfile
- using the _api-up recipe in more places
- converting the NGINX conf into a template to use env vars

Testing Instructions

Check out the PR and run the dev server.
Visit some endpoints that can help gauge the communication between processes e.g.
- the stats endpoint, to test ES and cache
- the thumbnails endpoint, to test the thumbnails service

Apart from this, a passing CI is pretty good proof of inter-process communication working normally.

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

…for-api`

…eam_db` service

github-actions · 2022-08-03T10:00:54Z

API Developer Docs Preview: Ready

https://wordpress.github.io/openverse-api/_preview/844

Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again.

You can check the GitHub pages deployment action list to see the current status of the deployments.

api/run.sh

justfile

Co-authored-by: sarayourfriend <[email protected]>

sarayourfriend

LGTM! Tested fine locally. Only one of the suggestions I'm strong-ish on is mapping the Postgres and Redis ports. Otherwise this is great.

api/docs/guides/test.md

docker-compose.yml

justfile

docker-compose.yml

sarayourfriend

LGTM!

dhruvkb · 2022-08-09T15:56:52Z

@obulat could you please review the PR when you have some time? Thanks!

krysal · 2022-08-09T16:09:37Z

@dhruvkb After Olga's review, be sure to merge this PR after the v2.5.10 is cut, please. This is looking like a non-trivial change.

sarayourfriend · 2022-08-09T18:18:14Z

Good point @krysal. Should just hold off on merging this at all until the ES situation is resolved? We sort of agreed to not merge anything that wasn't directly related to it. We did merge some other PRs I guess though, but they were not modifying the API code... 🤔

dhruvkb · 2022-08-10T03:55:17Z

We did agree to no merge anything unrelated to ES but my interpretation was to not touch the ES-related bits. I can definitely hold off on merging this but this only affects the dev setup, there is no change to the code itself.

I'll wait for the v2.5.10 to be cut before merging and block it till then.

sarayourfriend · 2022-08-10T12:26:32Z

We did agree to no merge anything unrelated to ES but my interpretation was to not touch the ES-related bits. I can definitely hold off on merging this but this only affects the dev setup, there is no change to the code itself.

Sure. Part of the issue is that we don't know what effects ES 🙂 At least that is one of the fundamental take-aways for myself over the last three months.

This is probably fine, so yeah, blocking until we have the current release cut with the changes we explicitly want to make seems like a good compromise to me. I personally do want this PR in sooner than later because the problem is a thorn in my side. I appreciate you working on it!

dhruvkb · 2022-08-15T12:45:20Z

@obulat could you please review this? I am aiming to merge this since 2.5.10 has been cut and deployed to production.

AetherUnbound

Looks and works great!

AetherUnbound · 2022-08-16T15:17:58Z

.github/workflows/ci_cd.yml

+    name: Check for API consumer docs
+    runs-on: ubuntu-latest
+    needs:
+      - build-images
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v2
+
+      - name: Setup just
+        uses: extractions/setup-just@v1
+
+      - name: Download all images
+        uses: actions/download-artifact@v2
+        with:
+          path: /tmp
+
+      - name: Load all images
+        run: |
+          docker load --input /tmp/api/api.tar
+          docker load --input /tmp/ingestion_server/ingestion_server.tar


I don't know if this is possible with GHA, but it would be really cool to have this as a set of steps that we could just reference as a precursor to specific tasks! Since this does appear to be reused elsewhere in this file.

It is possible using YAML anchors but actionlint does not support them yet: rhysd/actionlint#133

It's also possible with composite actions as we use in the frontend repository.

api/docs/guides/test.md

dhruvkb added 10 commits August 3, 2022 15:04

Remove all instances of wait-for-es because it's included in `wait-…

a2d0c09

…for-api`

Document and use _api-up shorthand in more places

f0d1429

Use NGINX templates instead of hardcoded port

4b811cc

Unbind ports used for internal communication

9065a0a

Remove redundant, unused settings

7021c82

Replace cURL command with just recipe

3ccf720

Remove unnecessary @ from recipe

f4ec488

Remap ports to the non-conflicting 502xx range

7fcf722

Update tests based on changes to the Docker Compose file

68cf98f

Make the ingestion_server and indexer_worker depend on the `upstr…

d0cedb7

…eam_db` service

dhruvkb added 🟨 priority: medium Not blocking but should be addressed soon ✨ goal: improvement Improvement to an existing user-facing feature 🤖 aspect: dx Concerns developers' experience with the codebase labels Aug 3, 2022

sarayourfriend reviewed Aug 3, 2022

View reviewed changes

api/run.sh Outdated Show resolved Hide resolved

justfile Show resolved Hide resolved

dhruvkb and others added 3 commits August 3, 2022 20:26

Split complex line into two

6df39aa

Co-authored-by: sarayourfriend <[email protected]>

Drop defunct testlocal functionality

1e8665e

Spin off ReDoc test into its own job

ad6a807

dhruvkb marked this pull request as ready for review August 4, 2022 08:52

dhruvkb requested a review from a team as a code owner August 4, 2022 08:52

dhruvkb requested review from obulat and sarayourfriend August 4, 2022 08:52

sarayourfriend suggested changes Aug 4, 2022

View reviewed changes

api/docs/guides/test.md Show resolved Hide resolved

docker-compose.yml Outdated Show resolved Hide resolved

docker-compose.yml Outdated Show resolved Hide resolved

justfile Outdated Show resolved Hide resolved

docker-compose.yml Outdated Show resolved Hide resolved

dhruvkb added 3 commits August 5, 2022 11:23

Change recipe dependencies into recipe steps

670dd8a

Describe how ports 8000 and 3000 are used

faa97ae

Map db and cache ports on host

d714b04

sarayourfriend approved these changes Aug 5, 2022

View reviewed changes

dhruvkb added the ⛔ status: blocked Blocked & therefore, not ready for work label Aug 10, 2022

dhruvkb removed the ⛔ status: blocked Blocked & therefore, not ready for work label Aug 11, 2022

AetherUnbound approved these changes Aug 16, 2022

View reviewed changes

dhruvkb merged commit 5326bfe into main Aug 17, 2022

dhruvkb deleted the remap_ports branch August 17, 2022 15:30

dhruvkb mentioned this pull request Feb 22, 2023

Increase separation of unit and integration tests WordPress/openverse#679

Open

1 task

dhruvkb mentioned this pull request Sep 25, 2022

Stop binding ports in the reservable range locally WordPress/openverse#200

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remap ports to the non-conflicting range #844

Remap ports to the non-conflicting range #844

dhruvkb commented Aug 3, 2022 •

edited

Loading

github-actions bot commented Aug 3, 2022 •

edited

Loading

sarayourfriend left a comment

sarayourfriend left a comment

dhruvkb commented Aug 9, 2022

krysal commented Aug 9, 2022

sarayourfriend commented Aug 9, 2022 •

edited

Loading

dhruvkb commented Aug 10, 2022 •

edited

Loading

sarayourfriend commented Aug 10, 2022

dhruvkb commented Aug 15, 2022 •

edited

Loading

AetherUnbound left a comment

AetherUnbound Aug 16, 2022

sarayourfriend Aug 16, 2022 •

edited

Loading

Remap ports to the non-conflicting range #844

Remap ports to the non-conflicting range #844

Conversation

dhruvkb commented Aug 3, 2022 • edited Loading

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

github-actions bot commented Aug 3, 2022 • edited Loading

sarayourfriend left a comment

Choose a reason for hiding this comment

sarayourfriend left a comment

Choose a reason for hiding this comment

dhruvkb commented Aug 9, 2022

krysal commented Aug 9, 2022

sarayourfriend commented Aug 9, 2022 • edited Loading

dhruvkb commented Aug 10, 2022 • edited Loading

sarayourfriend commented Aug 10, 2022

dhruvkb commented Aug 15, 2022 • edited Loading

AetherUnbound left a comment

Choose a reason for hiding this comment

AetherUnbound Aug 16, 2022

Choose a reason for hiding this comment

sarayourfriend Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

dhruvkb commented Aug 3, 2022 •

edited

Loading

github-actions bot commented Aug 3, 2022 •

edited

Loading

sarayourfriend commented Aug 9, 2022 •

edited

Loading

dhruvkb commented Aug 10, 2022 •

edited

Loading

dhruvkb commented Aug 15, 2022 •

edited

Loading

sarayourfriend Aug 16, 2022 •

edited

Loading