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

OCLOMRS-27: Adding Nginx Configuration to allow serving of different URL's in the single page application #14

Merged
merged 1 commit into from
May 24, 2018

Conversation

mwaz
Copy link
Contributor

@mwaz mwaz commented May 23, 2018

JIRA TICKET NAME:

OCLOMRS-27

Summary:

Currently the Nginx configuration does not allow serving different routes such as /dashboard or /login. The default.conf will overwrite the existing Nginx default.conf settings in the docker container and allow Nginx to serve files from all the supplied URL's. The Pull Request is in relation to the Ticket ITSM-4148.

@coveralls
Copy link

coveralls commented May 23, 2018

Pull Request Test Coverage Report for Build 65

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 42.391%

Totals Coverage Status
Change from base Build 55: 0.0%
Covered Lines: 34
Relevant Lines: 70

💛 - Coveralls

@cintiadr cintiadr self-assigned this May 23, 2018
@cintiadr
Copy link
Contributor

cintiadr commented May 23, 2018

I'm slightly confused by what exactly you want to achieve.
So, you are not able to change the the infrastructure nginx proxy. You cannot override or modify anything outside your docker container from inside your docker container, due to technical and logical restrictions.
Any application-specific logic should be done inside your docker container. I have more than 20 docker containers running now, your docker container should be responsible for any logic required by your application.

That said, nginx.conf is not a yaml file. It only works for the infrastructure nginx, which happens to be deployed using ansible (which parsers a yaml file). I copied the lines so you could have an idea what our proxy is using.

What you need to do, instead, is changing the file /etc/nginx/conf.d/default.conf inside your docker container. I don't really understand how you are modifying your docker image without changing your docker file. I did build your docker image locally, and as expected it didn't work.

First, I'd strongly recommend you update the docker compose file, as I recommended before: #16

And then, I'd ask you to do:

# build docker image
$ docker build -t openmrs/ocl-client:local .

# start docker image
$ docker-compose up -d

# enter docker image
$ docker exec -it openmrs-ocl-client_oclclient_1 sh

Copy the file /etc/nginx/conf.d/default.conf to this repo. Edit the way you want.
Change your dockerfile to copy this file to your container.

To redo it:
$ docker-compose down -v

# build docker image
$ docker build -t openmrs/ocl-client:local .

# start docker image
$ docker-compose up -d

After you get it working inside your browser, locally, from inside docker, I think it would be good to go.

@cintiadr cintiadr self-requested a review May 23, 2018 10:31
Copy link
Contributor

@cintiadr cintiadr left a comment

Choose a reason for hiding this comment

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

Added on the previous comment

@mwaz
Copy link
Contributor Author

mwaz commented May 23, 2018

Thankyou @cintiadr I have made changes to the PR as requested.

@cintiadr
Copy link
Contributor

It works! 💃

When I run it locally, http://localhost:8081/dashboard loads.

This PR is good as is, but I'd recommend for next changes/PRs:

  • Can you add on the README how to use docker/docker-compose locally? I could do it too, but I have the feeling you'd be able to write it from a beginners' perspective, making the README more... readable.
  • If you want, and only if you want, you could move the default.conf and docker-compose file to a folder; usually it's called like docker, for files specifically used for docker. Moving the dockerfile is possible, but it's a little boring. It's just organisation, it doesn't really change functionality. Note: if you move the docker-compose.yaml file, you need to use -f option in docker-compose command to find the file
  • Again, if you want, you could delete all the comments from inside your default.conf file. It doesn't affect at all.

@mwaz
Copy link
Contributor Author

mwaz commented May 24, 2018

I have updated the PR with requested changes @cintiadr

@emasys
Copy link
Contributor

emasys commented May 24, 2018

LGTM

Copy link
Contributor

@EleisonC EleisonC left a comment

Choose a reason for hiding this comment

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

LGTM

@cintiadr cintiadr merged commit d70192c into openmrs:master May 24, 2018
Copy link
Collaborator

@hadijahkyampeire hadijahkyampeire left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants