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

Nginx docker temp #700

Closed
wants to merge 17 commits into from
Closed

Nginx docker temp #700

wants to merge 17 commits into from

Conversation

ZacharyCChang0828
Copy link
Contributor

I moved the entire nginx configuration file into the repository and modified docker-compose so it now use it as it's volume.
blockips.conf is added to specify IPs to block.
1_webapp.conf contains rate limiting and configuration to connect nginx to the app.

@auspicacious
Copy link
Contributor

@ZacharyCChang0828 Thank you, I will try to look through it this afternoon.

Related to issue #648

@@ -0,0 +1,47 @@
#Modified from: https://itnext.io/docker-rails-puma-nginx-postgres-999cd8866b18
Copy link
Contributor

Choose a reason for hiding this comment

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

About the filename: files are sorted in lexical order, not numeric order, so this will not behave the way that you think it will.

01_file
02_file
09_file
10_file
1_file
11_file
50_file
99_file

The convention is to use two digits, and if the ordering doesn't matter, use 50_ to put it in the middle.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the other files aren't following the normal convention, so I guess it doesn't matter that much. 1_ will sort before anything that begins with a letter of the alphabet.

@@ -0,0 +1 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

What's supposed to be here?

@@ -0,0 +1 @@
client_max_body_size 10M;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can delete the .bak files to reduce the distraction.

@@ -0,0 +1,47 @@
#Modified from: https://itnext.io/docker-rails-puma-nginx-postgres-999cd8866b18

upstream safecastapi {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would appreciate it if you could leave a comment on this pull request to describe where you made changes and what changes you made.

server app:3000;
}

limit_req_zone $binary_remote_addr zone=one:10m rate=10r/s;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a good start, but, if it is possible, it would be best if we could add this configuration to a new file instead of modifying an existing one.

@auspicacious auspicacious marked this pull request as draft June 18, 2020 05:32
@matschaffer
Copy link
Contributor

Closing out since this has been draft for quite some time and both folks involved aren't active at the monent.

@eitoball eitoball deleted the nginx_docker_temp branch August 14, 2022 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants