-
Notifications
You must be signed in to change notification settings - Fork 281
Use per-project Sentry configuration #4818
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
Changes from all commits
7ea9a8d
da6ec1e
226090a
7b5971e
5ad0072
8c356f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,10 @@ plugins { | |
| alias(libs.plugins.compose.compiler) | ||
| } | ||
|
|
||
| sentry { | ||
| projectName = project.findProperty("sentryWearProject")?.toString() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MiSikora question, just to double check: I don't think there should be differences related to what we do in annotate_sentry_mapping_uuid given we're building separate app bundles anyway (that is, each app will have its own
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question but I don't know. I would think that no changes are required since |
||
| } | ||
|
|
||
| android { | ||
| namespace = "au.com.shiftyjelly.pocketcasts" | ||
|
|
||
|
|
@@ -32,8 +36,8 @@ android { | |
| named("release") { | ||
| manifestPlaceholders["appIcon"] = "@mipmap/ic_launcher" | ||
|
|
||
| if (!file("${project.rootDir}/sentry.properties").exists()) { | ||
| println("WARNING: Sentry configuration file 'sentry.properties' not found. The ProGuard mapping files won't be uploaded.") | ||
| if (project.findProperty("sentryWearProject")?.toString().isNullOrBlank()) { | ||
| println("WARNING: Sentry configuration not found. The ProGuard mapping files won't be uploaded.") | ||
| } | ||
| } | ||
| } | ||
|
|
||
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.
If I'm not mistaken, those 2 properties as still in
sentry.propertiescurrently, notsecret.properties.sentry.propertiesfile as-is (and also in.configure) and let theSentryPluginExtensionautomatically read those (at least I'm guessing that's who reads those implicitly so far) fromsentry.properties, only overwriting theprojectNamevia Gradle (assuming setting asentry { … }section in thebuild.gradle.ktsfiles doesn't disableSentryPluginExtensionreading fromsentry.propertiesimplicitly, but instead adds on top of it so that the properties are merged?)applyCommonSentryConfiguration, and want to move secret values fromsentry.propertiestosecrets.propertiesto have Gradle read from that.propertiesfile instead… which could make sense too… but in that case I'd expect this PR to contain changes for the.configure-files/secrets.properties.enc, deletion of the.configure-files/sentry.properties.encand removal of the entry forsentry.propertiesin the.configureJSON file to reflect thatPS: ICYMI, the convoluted/confusing way we currently use to manage/updates secrets in git repos with
configure(and how to update the.encfiles when you change secrets, etc) will very soon change to become way more simple and transparent (Internal ref: paaHJt-96q-p2). 🔜Uh oh!
There was an error while loading. Please reload this page.
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.
Thanks for bringing this up. This might have been unclear from the PR description but this is what I meant.
I didn't want to update secrets in case the approach I'm proposing is incorrect. And yes, this approach means dropping
sentry.properties.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.
Ah, un-caffeinated me missed that part, thanks for highlighting it! Makes sense indeed.
If this PR is not extra-urgent and can wait a couple of days, maybe I can put PCAndroid as next on the list in my project to test our brand new way to manage in-repo secret files (AINFRA-1539).
Since that new approach to deal with in-repo secrets does not rely on
.mobile-secretsnor.configureanymore, and instead transparently handles secret files via git's built-in filters feature, that means that any change you need to make on a secret files will then be done directly in the PCAndroid repo—and thus be part of the list of files changed in the PRs, with the contents of the secret file not being updated inmainuntil the PR is merged there, like any other regular file, etc.So once we adopt that new tool you wouldn't have the problem above in this PR anymore.
If you don't want to wait for me to migrate PCAndroid to the new tool and want to merge this PR earlier that's ok with me too, though in that case you'll indeed have to synchronize your merging of this PR with the push of changes to secret files in
.mobile-secrets.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.
No, I think it can wait. I'd be great to test using git filters for that. I'll keep it open for now but as a draft. Thanks!