Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: have two apache servers #10766

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

chore: have two apache servers #10766

wants to merge 6 commits into from

Conversation

alexgarel
Copy link
Member

@alexgarel alexgarel commented Sep 3, 2024

@alexgarel alexgarel requested a review from a team as a code owner September 3, 2024 16:18
@alexgarel alexgarel marked this pull request as draft September 3, 2024 16:18
@github-actions github-actions bot added the 🪶 Apache We use Apache as a server to run Open Food Facts label Sep 3, 2024
default 8005;

# home pages
"~*GET/$" 8004;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should handle also the HEAD and OPTIONS methods (used by browsers for CORS)

"~*POST/cgi/product.pl" 8004;
# product API read / write
"~*GET/api/v./product/" 8004;
"~*POST/api/v./product/" 8004;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For API v3, we use the PATCH method too

@@ -13,6 +13,21 @@ server {
}
}

# map to decide if we go to the main (8004) or secondary service (8005)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"secondary" sounds like it's not a priority. I almost expected that we would send slow requests that we care a bit less about to the "secondary" Apache.

I think it would be clearer to use a name that gives the intent, like "fasttrack" or "fastlane".

env/env.obf.main Outdated
@@ -0,0 +1,3 @@
# This is used by ports.conf in apache2
APACHE_ARGUMENTS=-D APACHE2_PORT=8002
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are changing those files, could we use the same port numbers for all flavors? We don't run them in the same server anymore, so we could just use 8001 (and 8002 if there's a fast track Apache) for all flavors.

Copy link
Contributor

@stephanegigandet stephanegigandet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thank you.

I added a few comments, I think we should at least add the HEAD/OPTIONS/PATCH methods (or maybe just remove the method altogether, I can't think of a case where we would want to send a request to different Apache servers based on the method)

Also unify port number to 8001
@alexgarel alexgarel marked this pull request as ready for review September 4, 2024 21:26
@github-actions github-actions bot added 🧴 Open Beauty Facts Our cosmetic analysis project https://world.openbeautyfacts.org 🐾 Open Pet Food Facts Our pet food analysis project https://world.openpetfoodfacts.org 📸 Open Products Facts Our project to increase the lifespan of objects. https://world.openproductsfacts.org labels Sep 4, 2024
Copy link

sonarcloud bot commented Sep 6, 2024

@github-actions github-actions bot added the 💥 Merge Conflicts 💥 Merge Conflicts label Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪶 Apache We use Apache as a server to run Open Food Facts 💥 Merge Conflicts 💥 Merge Conflicts NGINX 🧴 Open Beauty Facts Our cosmetic analysis project https://world.openbeautyfacts.org 🐾 Open Pet Food Facts Our pet food analysis project https://world.openpetfoodfacts.org 📸 Open Products Facts Our project to increase the lifespan of objects. https://world.openproductsfacts.org
Projects
Status: In progress
Status: Review in progress
Status: In progress
Development

Successfully merging this pull request may close these issues.

2 participants