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

Make dumpus API easier to self-host #48

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Priultimus
Copy link

Hello! I like your project, and I was trying to self host the API. I wasn't able to find great documentation on how to use caprover, so I figured I'd use Docker. It was pretty easy, so I thought I'd add a docker-compose.yml file to the project, just to save any others who might pass by some time.

I also took a look at your project diswho, and it looked like it was just a wrapper around making a GET request to Discord. I figured, you could also add the option to use your own Discord secret and have the API make a request directly to Discord rather than installing a whole other project just to do so.

Now, to install, you only have to install docker, put a DISCORD_SECRET or DISWHO_BASE_URL and DISWHO_JWT_SECRET into your .env, and then run docker-compose build && docker-compose up and it should just work :)

Hopefully this doesn't break anything! 😅 Thank you for your time, and I do really like the project.

@Priultimus
Copy link
Author

Also! A note: I did attempt to include flower in the docker-compose, but I ultimately commented it out, since it required me to make changes to Dockerfile.flower (it also needs to copy and run the download-ntk.py script), but as I haven't used caprover I don't know if you left it out for a specific reason, and I didn't want to intrude or break anything!

@Androz2091
Copy link
Member

Thank you for your PR, I will review this as soon as possible!

Copy link

@builtbyvys builtbyvys left a comment

Choose a reason for hiding this comment

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

This is a great PR, although there are some minor things from the cherry tree that I feel should be acknowledged. Thank you for this, I was also stuck on getting the API up and going!

Comment on lines 15 to 17
environment:
RABBITMQ_DEFAULT_USER: dumpus
RABBITMQ_DEFAULT_PASS: dumpus

Choose a reason for hiding this comment

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

Here, it would be better if you used environment variables instead of hard coding usernames and passwords 😅

environment:
  RABBITMQ_DEFAULT_USER: ${RABBITMQ_USER}
  RABBITMQ_DEFAULT_PASS: ${RABBITMQ_PASSWORD}

dockerfile: Dockerfile.worker

healthcheck:
test: [ "CMD-SHELL", "celery", "-A", "tasks", "inspect", "ping", "-d", "regular_process@$HOSTNAME" ]

Choose a reason for hiding this comment

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

test: ["CMD-SHELL", "celery -A tasks inspect ping -d regular_process@$$HOSTNAME"]

This is a better way to implement the health check because the command isn't split into multiple arguments but rather a single shell command.

RABBITMQ_DEFAULT_USER: dumpus
RABBITMQ_DEFAULT_PASS: dumpus

restart: unless-stopped

Choose a reason for hiding this comment

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

Consider on-failure in case of an error

POSTGRES_PASSWORD: dumpus
POSTGRES_DB: dumpus

restart: unless-stopped

Choose a reason for hiding this comment

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

Consider on-failure in case of an error

retries: 5

environment:
POSTGRES_USER: dumpus

Choose a reason for hiding this comment

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

Same thing as the first comment, use environment variables

Comment on lines 59 to 61
environment:
POSTGRES_URL: "postgresql+psycopg2://dumpus:dumpus@db:5432/dumpus"
RABBITMQ_URL: "amqp://dumpus:dumpus@broker:5672/"

Choose a reason for hiding this comment

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

Make sure to update the connection strings

Comment on lines 75 to 77
environment:
POSTGRES_URL: "postgresql+psycopg2://dumpus:dumpus@db:5432/dumpus"
RABBITMQ_URL: "amqp://dumpus:dumpus@broker:5672/"

Choose a reason for hiding this comment

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

Make sure to update the connection strings

Comment on lines +21 to +25
db:
container_name: dumpus-db
image: postgres:latest
ports:
- "6641:5432"

Choose a reason for hiding this comment

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

Adding a volume here would be useful for database persistence

db:
  volumes:
    - postgres_data:/var/lib/postgresql/data
volumes:
  postgres_data:

@builtbyvys
Copy link

@Priultimus, can I apply the suggested changes via a PR to your fork? just wanna make sure that's okay with you :D

@Androz2091
Copy link
Member

Thank you for your review work @builtbyvys
Do you also have personal suggestions about issues you encountered during the setup?

@builtbyvys
Copy link

Thank you for your review work @builtbyvys
Do you also have personal suggestions about issues you encountered during the setup?

The issues I was encountering were just missing context when it came to deploying this, and an absence of documentation overall. I understood it when I saw the PR, and got up and running shortly after successfully.

Good project though, thank you for your work!

@Priultimus
Copy link
Author

@builtbyvys Yeah that would be great, thanks!

@builtbyvys
Copy link

just bumping due to inactivity, this should be ready to merge!

@Androz2091
Copy link
Member

Thank you, I will review this asap!

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