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

[Sentry Bundle] Make config easier to use and deploy-friendly #348

Closed
wants to merge 2 commits into from
Closed

[Sentry Bundle] Make config easier to use and deploy-friendly #348

wants to merge 2 commits into from

Conversation

msheakoski
Copy link

@msheakoski msheakoski commented Apr 16, 2018

I made a few changes to the config to remove some frustrations that I encountered while developing and deploying. They were inspired by the Doctrine recipe:

  • Set a default value for the SENTRY_DSN env var to prevent errors during deployment when running commands like cache:warmup
  • Merge config/packages/prod/sentry.yaml into config/packages/sentry.yaml for simplicity. I do not see a reason for separating them by default

Sentry will be enabled in any environment where a non-blank SENTRY_DSN env var is provided. Not setting the var or having it set to an empty string will disable Sentry error handling

@ruudk pinging you since you are the maintainer

Q A
License MIT

* Set a default value for the SENTRY_DSN env var to prevent errors during deployment
* Merge config/packages/prod/sentry.yaml into config/packages/sentry.yaml
* Sentry will be enabled in any environment where a non-blank SENTRY_DSN env var is provided
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

@@ -1,4 +1,13 @@
parameters:
Copy link

Choose a reason for hiding this comment

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

"parameters" should be defined via the "container" configurator instead

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not very helpful. I have no idea what it means. Maybe it would be great to include a link to the docs describing this? /cc @fabpot

@ruudk
Copy link
Contributor

ruudk commented Apr 16, 2018

Set a default value for the SENTRY_DSN env var to prevent errors during deployment when running commands like cache:warmup

What issues do you get?

You just need to add the SENTRY_DSN= to your .env file.

When you install this recipe, it will automatically do this for you.

@msheakoski
Copy link
Author

You just need to add the SENTRY_DSN= to your .env file.

Sure, it works fine in the "dev" environment, but when you want to deploy your app and have steps in your deployment script that warm up the cache you receive Environment variable not found: "SENTRY_DSN" errors. I am simply copying the pattern used for the Doctrine recipe since they encountered and solved the same issue.

@msheakoski
Copy link
Author

@ruudk please see symfony/recipes#258 for their discussion on the same issue that the Sentry bundle faces.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

@@ -1,4 +1,12 @@
parameters:
Copy link

Choose a reason for hiding this comment

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

"parameters" should be defined via the "container" configurator instead

@Nyholm
Copy link
Member

Nyholm commented May 13, 2018

Thank you for this PR.

I do not agree with the reasons you specify here. I would very much like that the deployment process fails if I do not have all my environment variables set. This issue as you refer to is not specific to Sentry but it is how environment variables work.

Im currently 👎

@ruudk
Copy link
Contributor

ruudk commented May 14, 2018

@Nyholm I agree,

@Nyholm
Copy link
Member

Nyholm commented Jun 22, 2018

@msheakoski, what do you think?

@msheakoski
Copy link
Author

I would very much like that the deployment process fails if I do not have all my environment variables set.

@Nyholm @ruudk this is a good point. I will close this PR since the current behavior seems to be the best approach.

@msheakoski msheakoski closed this Jul 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants