-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Improve setup #123
Improve setup #123
Conversation
This line in the README really confused me. 👍 to this PR. |
docker-compose.override.yml | ||
|
||
# env config | ||
.env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps use absolute paths: /.env
, /docker-compose.override.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rest of the env will be using relative paths, perhaps should then update entire .gitignore?
updated a bit - did mostly mean that removed all outdated info about it from readme
|
+1 for env file instead of hardcode |
@dcramer @mattrobenolt Would you be able to review this, and merge if possible? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this cause problems for existing users with stateful data? I’m assuming the volumes are an accepted thing, but I have very limited personal experience running docket images.
Is there a migration path we need to suggest to people?
its not less damaging than #105 - also i'm fairly sure all current users are using some form of fork or copy of current setup to actually run sentry (as current setup is not usable 1:1 - you need to tweak it a bit -> https://github.com/getsentry/onpremise/blob/ea47a6283f089d0f5c8202fed7b573e11edd8122/docker-compose.yml#L23 ) with changes from this pull - it would be at-least possible to simply use this setup with some locally customized configs on top of it actually run sentry using this repo setup directly. |
So I am not exactly opposed to this, and I don't particularly attempt to maintain compatibility for older versions, because as you mentioned, you should be forking and editing this on your own anyways. With that said, my comprehension of this docker-compose file is going beyond my comfort zone since I don't personally use docker-compose, and never intended this file to be used this way. I originally added this file as literally a So, I have no aversion to merging this. I just have less understanding of this since I don't follow docker-compose best practices, or frankly, how people even use this in production since we don't, and I would never think to use compose as a production tool. If this is the direction that the community recommends and this is leading best practices, well, then that's fine with me and I have no other opinions other than I am able to personally support this less and less since I don't know this tool well. |
This reverts commit e21d9a3.
* declare sentry volumes external * move SENTRY_SECRET_KEY to .env file (not in git repo) * ignore docker-compose.override.yml to allow extending
This pull request improves upon #105 ( and follows for rejected #115 )
docker-compose down -v
)SENTRY_SECRET_KEY
to .env file that is not part of git repo (so setup from here can be used directly)docker-compose.override.yml
to .gitignore file ( so base setup can be extended, I, for example, used that to add a label to web container to get traefik routing for https )