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

Added Proper Docker Compose and .env.docker-setup files #14724

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

phil-flip
Copy link

Description

For a while now @svpernova09 and I have been planning on rewriting the Docker Docs so it uses Docker compose, instead of Docker run. That requires us to change some stuff in the repo. Like having a docker-compose.yml and .env.docker file which can be used by the user.

This Pr should help with getting less support inquiries in the Discord Docker channel and remove docker run from production deployments.

Thankies to @svpernova09 for helping me brainstorm about possible solutions and providing the health checks for the docker-compose.yml

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

The updated article can be read in the docs.
But I tested my setup, by running the following commands.

  • Clone Repo
  • Rename .env.docker to .env
  • Change port in .env, if needed
  • Run docker compose up -d and wait for startup
  • Add Content
  • Run docker compose down to remove deployment, but not the volumes
  • Run docker compose up -d and wait for startup
  • Check if content is still there
  • Run docker compose down -v delete deployment and data

Checklist:

@phil-flip phil-flip requested a review from snipe as a code owner May 16, 2024 00:43
Copy link

welcome bot commented May 16, 2024

💖 Thanks for this pull request! 💖

We use semantic commit messages to streamline the release process and easily generate changelogs between versions. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix if it doesn't have one already.

Examples of commit messages with semantic prefixes:

  • Fixed #<issue number>: don't overwrite prevent_default if default wasn't prevented
  • Added #<issue number>: add checkout functionality to assets
  • Improved Asset Checkout: use new notification method for checkout

Things that will help get your PR across the finish line:

  • Document any user-facing changes you've made.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

Copy link

what-the-diff bot commented May 16, 2024

PR Summary

  • Introduction of a new .env.dev.docker file
    This new file holds various settings for fine-tuning the application, including those related to database setup, application settings, file storage, mail server, image library, backup policies, session management, security measures, cache operations, Redis configurations, AWS settings, login throttling, and miscellaneous ones.

  • Alterations to .env.docker file
    The current .env.docker file has been updated to optimally configure the application for a production environment. The app now runs on port 8000. New parameters for the database root password and app settings that enable deletion of backups and data purging have been added. Alterations were also made to the database and Redis host settings.

  • Addition of dev.docker-compose.yml file
    A new file to run the application in a development environment using Docker Compose has been added. It comprises services for the main application 'snipeit', database 'mariadb', 'redis' for in-memory data structure store, and 'mailhog' as a mail testing tool.

  • Updates to docker-compose.yml file
    The primary Docker Compose file has been modified. Volumes have been added for improved data management. The main 'snipeit' service now uses a specific version of 'snipe-it' image. Restart policies have been established to maintain continuous service availability. Data storage for the application and database have been assigned to 'storage' and 'db_data' respectively. A health check function for our database service is now implemented for monitoring its performance. Finally, updates have been done to the environment variables.
    ``

@snipe snipe changed the title FEAT - Add Proper Docker Compose and .env.docker-setup files Added Proper Docker Compose and .env.docker-setup files May 16, 2024
@snipe snipe requested review from uberbrady and jerm May 16, 2024 13:52
Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

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

This looks AWESOME. Since we're so close to v7, I think I'm going to look at merging this once we do the v7->develop merge. If you want to get in front of that, you can definitely re-target to the snipeit_v7_laravel10 branch. But if you don't, I'm going to take it anyway - because this looks great and what I was hoping for. Thank you!

@phil-flip
Copy link
Author

phil-flip commented May 16, 2024

Thankies for the compliments. ^^
I had a feeling that because of the upcoming release there are not going to be made a lot of changes to v6. Tho I would prefer to merge it now as it should make a difference, unless the application stack changes (different DB, other backend, etc...)

Copy link
Collaborator

@jerm jerm left a comment

Choose a reason for hiding this comment

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

This looks great, but for the CIDR comment i made.

@@ -91,7 +96,7 @@ API_TOKEN_EXPIRATION_YEARS=40
# --------------------------------------------
# OPTIONAL: SECURITY HEADER SETTINGS
# --------------------------------------------
APP_TRUSTED_PROXIES=192.168.1.1,10.0.0.1
APP_TRUSTED_PROXIES=192.168.1.1,10.0.0.1,172.0.0.0/8
Copy link
Collaborator

@jerm jerm May 16, 2024

Choose a reason for hiding this comment

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

This should be 172.16.0.0/12.... not all 172 addresses are private 😉

Reference: https://www.rfc-editor.org/rfc/rfc1918#section-3

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also change the 192.168 and 10 blocks to full CIDR ranges as well?

Copy link
Author

Choose a reason for hiding this comment

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

This should be 172.16.0.0/12.... not all 172 addresses are private 😉

Reference: https://www.rfc-editor.org/rfc/rfc1918#section-3

Docker uses a /8 Network tho.

Copy link
Author

Choose a reason for hiding this comment

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

Should we also change the 192.168 and 10 blocks to full CIDR ranges as well?

To this day I still don't know if Docker uses 172/8 or 192.168/12 at work we had issues with the second range colliding with existing networks, which was strange to see, cause to that point I always thought they used the first one (172/8)

Copy link
Author

Choose a reason for hiding this comment

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

I rather wish to have the underlying hostname issue fixed, rather opening up that big of a range. But I am unsure if that is possible.

@uberbrady uberbrady added this to the Snipe-IT v7 milestone May 22, 2024
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.

None yet

3 participants