-
-
Notifications
You must be signed in to change notification settings - Fork 446
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: Allow setting proguard via Options and/or external resources #1728
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1728 +/- ##
============================================
+ Coverage 75.05% 75.19% +0.13%
- Complexity 2115 2126 +11
============================================
Files 212 212
Lines 7585 7606 +21
Branches 803 807 +4
============================================
+ Hits 5693 5719 +26
+ Misses 1504 1496 -8
- Partials 388 391 +3
Continue to review full report at Codecov.
|
@@ -286,7 +286,7 @@ private void mergeDebugImages(final @NotNull SentryEvent event) { | |||
|
|||
for (String item : proguardUUIDs) { | |||
DebugImage debugImage = new DebugImage(); | |||
debugImage.setType("proguard"); | |||
debugImage.setType(DebugImage.PROGUARD); |
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.
since MainEventProcessor
now applies the uuid to the event reading from the options, what's about setting the proguardUUID here to the options and let the MainEventProcessor create the DebugImage?
we only do it if the options isn't set though
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.
the other thing to consider is, event processor runs for every event, reading the uuid is a 1-time thing, so maybe it'd also make sense to move this logic from here to somewhere else eg AndroidOptionsInitializer
, the downside is that we do IO in the main thread, maybe we keep this refactor for another PR, we just need to tackle the comment above
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.
Here it reads a list of uuids (from all the resources), so it cannot be set on the options, or we change it here to single UUID.
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.
we can get just a single id, there's even a comment about that.
@marandaneto take a look please if reading order makes sense. Currently first the Proguard UUID is read from the manifest, if not found from |
@@ -106,10 +97,6 @@ public DefaultAndroidEventProcessor( | |||
|
|||
private @NotNull Map<String, Object> loadContextData() { | |||
Map<String, Object> map = new HashMap<>(); | |||
String[] proguardUUIDs = getProguardUUIDs(); | |||
if (proguardUUIDs != null) { | |||
map.put(PROGUARD_UUID, proguardUUIDs); |
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.
var PROGUARD_UUID
can be removed from this class now
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, missed that.
📜 Description
Allow setting proguard via Options and/or external resources.
UUIDs can be set on options either programmatically or using system properties, environment variables or
sentry.properties
file - same way as any other external property.💡 Motivation and Context
Fixes #1236
💚 How did you test it?
📝 Checklist