getsentry / docker-sentry

Docker Official Image packaging for Sentry
https://sentry.io
Other
349 stars 150 forks source link

Change sentry.conf.py to support custom schemas via environment variable. #166

Closed Wicked7000 closed 5 years ago

Wicked7000 commented 5 years ago

Hi,

I think in order to support users that want to run the docker image with a different schema then public you should add the 'SCHEMA' field to the postgres configuration and make it pull from an environment variable (Although keep it to default to public so that it wouldn't be a breaking change for people that want to stick with public).

Without this you get the issues highlighted in these github issues: https://github.com/getsentry/sentry/issues/2916 https://github.com/getsentry/sentry/issues/6588

postgres = env('SENTRY_POSTGRES_HOST') or (env('POSTGRES_PORT_5432_TCP_ADDR') and 'postgres')
if postgres:
    DATABASES = {
        'default': {
            'ENGINE': 'sentry.db.postgres',
            'NAME': (
                env('SENTRY_DB_NAME')
                or env('POSTGRES_ENV_POSTGRES_USER')
                or 'postgres'
            ),
            'USER': (
                env('SENTRY_DB_USER')
                or env('POSTGRES_ENV_POSTGRES_USER')
                or 'postgres'
            ),
       'SCHEMA': (
        env('SENTRY_DB_SCHEMA')
        or 'public'
        ),
            'PASSWORD': (
                env('SENTRY_DB_PASSWORD')
                or env('POSTGRES_ENV_POSTGRES_PASSWORD')
                or ''
            ),
            'HOST': postgres,
            'PORT': (
                env('SENTRY_POSTGRES_PORT')
                or ''
            ),
            'OPTIONS': {
                'autocommit': True,
            },
        },
    }
...
BYK commented 5 years ago

Hi! I tried to add this as an option but I don't think specifying 'public' as the default is equivalent to not specifying anything. This means we'd need to guard this behind an if statement, which means we need to double the tests to make sure each path is visited. This is quite costly to support a feature that is not widely needed and can easily be supported by just forking the config file in your setup.

So I'm going ahead and closing this as wontfix for now unless someone can come up with a simple way of supporting and maintaining this.