-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Rename onpremise to self-hosted #1169
Conversation
0f41be9
to
478b296
Compare
Mind giving this a once-over when you get a chance @aminvakil? Pretty sure there are no breaking changes here but would value a second opinion. |
@BYK Happy for your input as well if you're up for it. 🙇♂️ |
.env
Outdated
@@ -1,4 +1,4 @@ | |||
COMPOSE_PROJECT_NAME=sentry_onpremise | |||
COMPOSE_PROJECT_NAME=sentry_self_hosted |
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 trip people up if they are upgrading an existing installation?
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.
yes, this is likely a breaking change.
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.
In that case it seems as if we should keep this as sentry_onpremise
for now, put a note in the changelog for 21.12.0 and possibly a warning inline on install.sh
, and then make the breaking change in 22.1.0.
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.
It would be better to output this on install.sh
, users may probably not read the changelog on every release.
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.
One mention in our codebase:
self-hosted/install/wrap-up.sh
Lines 9 to 10 in 4593233
docker run --rm --network="${COMPOSE_PROJECT_NAME}_default" alpine ash \ | |
-c 'while [[ "$(wget -T 1 -q -O- http://web:9000/_health/)" != "ok" ]]; do sleep 0.5; done' |
... and then:
https://docs.docker.com/compose/reference/envvars/#compose_project_name
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.
How about not changing current onpremise
volumes and create self-hosted
in case there aren't already volumes created?
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.
Good news! We create most of our volumes by calling docker volume create
ourselves, marking them external
in docker-compose.yml
:
If set to
true
, specifies that this volume has been created outside of Compose.docker-compose up
does not attempt to create it, and raises an error if it doesn’t exist.
It seems that docker-compose
prepends the project name when creating a volume, but docker volume create
does not. 💃
$ docker volume list
DRIVER VOLUME NAME
$ ./install.sh --no-user-prompt
[...]
$ docker volume list
DRIVER VOLUME NAME
local f1d00624e9452e7075f7224f8eeb08744005c64938d6d2d92812d463926429b8
local sentry-clickhouse
local sentry-data
local sentry-kafka
local sentry-postgres
local sentry-redis
local sentry-symbolicator
local sentry-zookeeper
local sentry_onpremise_sentry-clickhouse-log
local sentry_onpremise_sentry-kafka-log
local sentry_onpremise_sentry-secrets
local sentry_onpremise_sentry-smtp
local sentry_onpremise_sentry-smtp-log
local sentry_onpremise_sentry-zookeeper-log
$
Sooooooooo ... can we get away with orphaning the log/secrets/smtp volumes? 🧐
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.
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.
I read #291 as saying that these are ephemeral volumes that we can recreate on down/up (the reason for #291 was to avoid littering with random volumes, not to address an operational issue with non-persistent volumes).
I've confirmed that the prefixed volumes are ephemeral with this method:
install.sh
onmaster
dcm up
- Populate a couple events.
export SENTRY_DSN="foo"
sentry-cli send-event -m "[Random-Error] $(echo $RANDOM | md5sum | head -c 20; echo;)" > /dev/null 2>&1 &
dcm down
- switch to this branch
install.sh
dcm up
- Did events persist? Yes.
- Populate a few more events. Did it work? Yes.
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.
Okay! I'm ready to say that this change is non-breaking. I've pushed 2573e87 but otherwise I am ready to go with this PR. Last chance to stop me @BYK @aminvakil et al. :)
.env
Outdated
@@ -1,4 +1,4 @@ | |||
COMPOSE_PROJECT_NAME=sentry_onpremise | |||
COMPOSE_PROJECT_NAME=sentry_self_hosted |
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.
yes, this is likely a breaking change.
@chadwhitacre Thanks for doing this! |
478b296
to
e8785a0
Compare
Waiting for this for a new deployment! Thanks for the work guys |
Wait no more, @ayozemr! :) Now that this is merged, would you mind trying it out and reporting back whether it breaks anything for you? Some orphaned containers/volumes/network are expected, but the upgrade should otherwise be seamless. |
#796