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

Better set up for docker-compose #636

Merged
merged 1 commit into from
Oct 31, 2022
Merged

Conversation

juhoinkinen
Copy link
Member

The docker-compose setup has not been really used much. It is mentioned in the Docker Wiki page mostly as a step towards production environment. To make NGINX proxy requests to Annif, the NGINX configuration has been written in the annif/nginx/nginx.conffile, which quite uselessly clutters the annif directory.

However there is an alternate solution: creating the config file with Docker cmd from content written inline in the docker-compose.yaml.

Changes:

  • Remove annif/nginx directory with the config file
  • Create nginx.conf with Docker run command
  • Remove unnecessary bash container
  • Better name for Annif container

- Remove annif/nginx directory
- Write nginx.conf with Docker run command
- Remove unnecessary bash container
- Better name for Annif container
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

gunicorn_server:
image: quay.io/natlibfi/annif
annif_app:
image: quay.io/natlibfi/annif:latest
Copy link
Member

Choose a reason for hiding this comment

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

I see that this changed to :latest, was this intended?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the tag is latest by default; this just makes the behaviour more explicit.

@codecov
Copy link

codecov bot commented Oct 28, 2022

Codecov Report

Base: 99.58% // Head: 99.58% // No change to project coverage 👍

Coverage data is based on head (db5b572) compared to base (7ec5d64).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #636   +/-   ##
=======================================
  Coverage   99.58%   99.58%           
=======================================
  Files          87       87           
  Lines        5990     5990           
=======================================
  Hits         5965     5965           
  Misses         25       25           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

LGTM, although I didn't test it

@juhoinkinen
Copy link
Member Author

juhoinkinen commented Oct 31, 2022

On second thought, would it be better to remove also the docker-compose.yaml from the Annif repository and just show it in Wiki? If one wants to use Annif with Docker, it does not make sense to have to clone the repository for using the compose file.

@osma
Copy link
Member

osma commented Oct 31, 2022

My slight preference would be to keep it as a file in the main repo. I see your point, but I think it's better to keep the file in a visible place under version control, and also being a part of Annif releases.

@juhoinkinen juhoinkinen merged commit 9fcc72f into master Oct 31, 2022
@juhoinkinen juhoinkinen deleted the simplify-docker-compose-setup branch October 31, 2022 08:46
@juhoinkinen
Copy link
Member Author

I updated the Wiki page to align to these changes.

@osma
Copy link
Member

osma commented Oct 31, 2022

The NGINX proxy server link on that page seems outdated, probably it should be switched to https://nginx.org/en/docs/ or some other suitable resource

@juhoinkinen
Copy link
Member Author

The NGINX proxy server link on that page seems outdated, probably it should be switched to https://nginx.org/en/docs/ or some other suitable resource

Link switched to the up-to-date one.

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

Successfully merging this pull request may close these issues.

2 participants