Skip to content

Fix corner case when a value is integer#2000

Closed
d1ys3nk0 wants to merge 1 commit intogetsentry:masterfrom
d1ys3nk0:master
Closed

Fix corner case when a value is integer#2000
d1ys3nk0 wants to merge 1 commit intogetsentry:masterfrom
d1ys3nk0:master

Conversation

@d1ys3nk0
Copy link
Copy Markdown

@d1ys3nk0 d1ys3nk0 commented Feb 7, 2023

Thanks for your Pull Request 🎉

Please keep these instructions in mind so we can review it more efficiently:

  • Add the references of all the related issues/PRs in the description
  • Whether it's a new feature or a bug fix, make sure they're covered by new test cases
  • If this PR contains any refactoring work, please give it its own commit(s)
  • Finally, please add an entry to the corresponding changelog

Other Notes

  • We squash all commits before merging
  • We generally review new PRs within a week
  • If you have any question, you can ask for feedback in our discord community first

Description

Describe your changes:

In edge case when I had parameter release coming from my env containing all digits like 9687411 I ended up with TypeError: no implicit conversion of Integer into String. Today, finally we catched this bug.

@sl0thentr0py
Copy link
Copy Markdown
Member

@lysenkooo we expect the release to be a string throughout the SDK, this should be fixed at the source in ReleaseDetector.
How are you passing in the release in your env, via SENTRY_RELEASE?

@d1ys3nk0
Copy link
Copy Markdown
Author

d1ys3nk0 commented Feb 15, 2023

@lysenkooo we expect the release to be a string throughout the SDK, this should be fixed at the source in ReleaseDetector. How are you passing in the release in your env, via SENTRY_RELEASE?

We use this way:

  Sentry.init do |config|
    config.enabled_environments = %w[production staging development]
    config.dsn = Rails.application.secrets.sentry_dsn
    config.release = Rails.application.secrets.app_build

In that case app_build could be integer since it goes from ENV and being automatically converted.

In order to fix it now we started to use this:

config.release = Rails.application.secrets.app_build.to_s

But we have spent a month to catch this. Because it was occuring only when app_build (which is first 7 chars from commit hash actually) was containing only digits.

@sl0thentr0py
Copy link
Copy Markdown
Member

ok thanks, I'll modify the config.release= writer to throw a warning if release is not a string instead.

@sl0thentr0py
Copy link
Copy Markdown
Member

closing in favor of #2004

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.

2 participants