search

swp-fu-eid / eid-fu-swp

Docker-based REST API implemented with Django and restframework.
MIT License
2 stars 1 forks source link

Windows fix #23

Closed BenjaminKeller closed 7 years ago

BenjaminKeller commented 7 years ago

Disclaimer: This is a duplicate of #2. As the old pull request was merging into develop and was missing one bugfix, I opened a new one. It has to be approved by the responsibles, too.

Description

This branch contains fixes for running the example on Windows. See #1 for more information. All members which are forced to use Windows should review this.

Naming of compose files

This is the only comment from #2. Therefore I copied it. At now there exist two docker compose files docker-compose.yml and docker-compose.logging.yml. Looking at the file names the intention is not clear. I think we have to improve on that.

1. Solution: Renaming

docker-compose.yml -> docker-compose.syslogd.yml docker-compose.logging -> docker-compose.default.yml

2. Solution: Switch to default logging

Is there any reason we want to log with syslogd? I am not a docker specialist.

3. Solution: Remain

If there is a good reason for logging with syslogd we should appreciate that and get to used by it.

Result

I think the first solution is the best as it forces you to choose one and rename it.

Armagetron commented 7 years ago

@nils-wisiol Travis will be used as soon as this PR and #29 are both merged into master. Or do you want both PR merged before they are submitted into master?

BenjaminKeller commented 7 years ago

@nils-wisiol I cleaned the commits as instructed and moved the syslogd configuration to an additional file. The syslogd config can now be run by using:

docker-compose -f docker-compose.yml -f docker-compose.syslogd.yml up --build
nils-wisiol commented 7 years ago

@Armagetron I may be overlooking something, but wouldn't it be worth running this PR through the existing travis configuration? This PR modifies the www configuration, that's why the tests could be useful.

Other than that, this lgtm

Armagetron commented 7 years ago

@nils-wisiol Should we merge the travis config into this branch?

nils-wisiol commented 7 years ago

My bad - I somehow thought there was already a configuration in master that was just not active. Let's add the CI later then.