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

feat: Improve configuration to be less Docker-specific #167

Merged
merged 3 commits into from
Apr 26, 2019

Conversation

dcramer
Copy link
Member

@dcramer dcramer commented Apr 15, 2019

This removes auto-binding of various values (specified in config.yaml) when they're not actually configured. It ensures that these values can then be configured from the web UI as Sentry has intended.

See also getsentry/docker-sentry#163 (comment)

@dcramer
Copy link
Member Author

dcramer commented Apr 15, 2019

I haven't actually tested this out yet, just wanted to get this up before I get my day started.

@dcramer dcramer requested a review from mattrobenolt April 15, 2019 17:03
SENTRY_OPTIONS['mail.port'] = int(env('SENTRY_EMAIL_PORT') or 25)
SENTRY_OPTIONS['mail.use-tls'] = env('SENTRY_EMAIL_USE_TLS', False)
else:
SENTRY_OPTIONS['mail.backend'] = 'dummy'
Copy link
Member Author

Choose a reason for hiding this comment

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

ive intentionally removed this behavior as email is required for a production sentry install, and you can configure the smtp settings in the UI

@dcramer
Copy link
Member Author

dcramer commented Apr 15, 2019

Also still not 100% clear if I should PR this or just patch the 9.1 config in docker-sentry, but I assumed this was the source of truth.

@markotitel
Copy link

Hi,
I've tried to build sentry image using this branch. But docker logs tells !! Configuration error: ValueError: invalid literal for int() with base 10: ''

@dcramer
Copy link
Member Author

dcramer commented Apr 16, 2019

@markotitel thanks! Might have to make the value checks look for empty before coercing.

This removes auto-binding of various values (specified in config.yaml) when they're not actually configured. It ensures that these values can then be configured from the web UI as Sentry has intended.
@dcramer dcramer force-pushed the feat/improve-config branch from 038f968 to 41200b7 Compare April 16, 2019 16:04
@dcramer
Copy link
Member Author

dcramer commented Apr 16, 2019

@markotitel feel free to give the branch another try now

@gebv
Copy link

gebv commented Apr 16, 2019

Maybe forgot SECRET_KEY rename to SENTRY_SECRET_KEY?

@dcramer
Copy link
Member Author

dcramer commented Apr 16, 2019

@gebv should be in there -- make sure you do a hard reset as ive forced pushed any fixes

SENTRY_OPTIONS['slack.client-id'] = env('SLACK_CLIENT_ID')
SENTRY_OPTIONS['slack.client-secret'] = env('SLACK_CLIENT_SECRET')
SENTRY_OPTIONS['slack.verification-token'] = env('SLACK_VERIFICATION_TOKEN') or ''
def bind_env_config(config=SENTRY_OPTIONS, mapping=ENV_CONFIG_MAPPING):
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to worry about mutable default arguments here?

Copy link
Member Author

Choose a reason for hiding this comment

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

what do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, it isn't a relevant issue for this function.

@dcramer
Copy link
Member Author

dcramer commented Apr 17, 2019

Expanded config to include new github + vsts vars.

sentry.conf.py Outdated
'SENTRY_EMAIL_PASSWORD': 'mail.password',
'SENTRY_EMAIL_USER': 'mail.username',
'SENTRY_EMAIL_PORT': ('mail.port', int),
'SENTRY_EMAIL_USE_TLS': ('mail.use-tls', bool),
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to use sentry.utils.types:Bool otherwise this coersion is going to yield a very unexpected behavior and a change from what it was before. Doing env('...', False) with False as the default would cause it to use this Bool type becuase it gets passed through sentry.utils.types:type_from_value.

@dcramer dcramer merged commit 0b18430 into master Apr 26, 2019
@dcramer dcramer deleted the feat/improve-config branch April 26, 2019 16:47
@github-actions github-actions bot locked and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants