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

WIP: custom nginx port via env variable #35

Merged
merged 4 commits into from
Apr 26, 2018
Merged

WIP: custom nginx port via env variable #35

merged 4 commits into from
Apr 26, 2018

Conversation

empeje
Copy link
Member

@empeje empeje commented Jan 27, 2018

The number of changed files should be divisible by 4 since we have 4 version (x86 and arm for each Alpine and Debian) + 1 for readme 😉 .

It is an update for automatically detect nginx port

Solved #29

@empeje empeje requested review from Dashlorde and dogi January 27, 2018 16:41
@empeje empeje self-assigned this Jan 27, 2018
@empeje empeje added the bug label Jan 27, 2018
@empeje
Copy link
Member Author

empeje commented Jan 29, 2018

Upps, I found bug in alpine version. Please don't merge it yet, the start.sh need to be udpdated.

@@ -42,6 +42,8 @@ then
echo "env[MOODOLE_DB_PORT] = '$MOODOLE_DB_PORT'" >> $PHP_FM_CONFIG
fi

envsubst '$NGINX_PORT' < $NGINX_CONFIG.template > $NGINX_CONFIG
Copy link
Member Author

Choose a reason for hiding this comment

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

you should put it in the top in every version because we need to render the template before adding manipulation

@empeje
Copy link
Member Author

empeje commented Jan 29, 2018

Done solving the render template problem.

@xyb994
Copy link
Member

xyb994 commented Feb 3, 2018

@mappuji I tried with the following docker-compose file on Raspberry Pi and it worked fine. Do we need to change both ports: and NGINX_PORT part? What's the right way to serve moodle at port 8080?

version: '2'
services:
  moodledb_rpi:
    image: arm32v7/postgres
    container_name: moodledb_rpi
    environment:
    # MAKE SURE THIS ONE SAME WITH THE MOODLE
    - POSTGRES_DATABASE=moodle
    - POSTGRES_USER=moodle
    - POSTGRES_PASSWORD=moodle
  moodle_rpi:
    image: treehouses/moodle:rpi-alpine-0.0.2-nginx-73baae59
    container_name: moodle_rpi
    ports:
      - "8080:8080"
    environment:
    - NGINX_PORT=8080
    - MOODOLE_DB_URL=moodledb_rpi
    - MOODOLE_DB_NAME=moodle
    - MOODOLE_DB_USER=moodle
    - MOODOLE_DB_PASS=moodle
    - MOODOLE_DB_PORT=5432
    - MOODOLE_MAX_BODY_SIZE=200M
    - MOODOLE_BODY_TIMEOUT=300s

@empeje
Copy link
Member Author

empeje commented Feb 3, 2018

Yes, you nailed it @xyb994

@dogi
Copy link
Member

dogi commented Feb 3, 2018

@xyb994 congrats - looks like we nailed it ...

... but I am very confused @mappuji

and how do we convey to the users that they have to change all 3 numbers

@empeje
Copy link
Member Author

empeje commented Feb 3, 2018

Let me explain, this part

    - NGINX_PORT=8080

is actually doing the job to change the port. NGINX_PORT if leave empty will fallback to 80

but, this part

    ports:
      - "8080:8080"

is a way we do it in docker compose, we redirect (bind) port 8080 in container to port 8080 in the host machine.

@empeje
Copy link
Member Author

empeje commented Feb 6, 2018

What do you think @dogi ? Should I merge it or revise it?

@empeje
Copy link
Member Author

empeje commented Feb 13, 2018

@empeje
Copy link
Member Author

empeje commented Feb 18, 2018

It is a bad idea to have a custom port, the better approach is bind the nginx server to 0.0.0.0 then redirect using docker compose feature.

@empeje
Copy link
Member Author

empeje commented Feb 28, 2018

After trial and error with #39

@empeje
Copy link
Member Author

empeje commented Apr 26, 2018

I think I'll just merge this one, will figure out if there unexpected issues.

@empeje empeje merged commit b2b9318 into master Apr 26, 2018
@empeje empeje deleted the nginx branch September 16, 2018 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants