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: Allow setting proguard via Options and/or external resources #1728

Merged
merged 10 commits into from
Sep 20, 2021

Conversation

maciejwalkowiak
Copy link
Contributor

@maciejwalkowiak maciejwalkowiak commented Sep 16, 2021

📜 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

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

@maciejwalkowiak maciejwalkowiak marked this pull request as ready for review September 16, 2021 10:05
@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2021

Codecov Report

Merging #1728 (e026dc3) into main (ae95ea3) will increase coverage by 0.13%.
The diff coverage is 80.95%.

Impacted file tree graph

@@             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     
Impacted Files Coverage Δ
...y/src/main/java/io/sentry/protocol/DebugImage.java 21.87% <ø> (+18.75%) ⬆️
sentry/src/main/java/io/sentry/SentryOptions.java 85.47% <66.66%> (-0.32%) ⬇️
...ry/src/main/java/io/sentry/MainEventProcessor.java 83.89% <86.66%> (+0.40%) ⬆️
sentry/src/main/java/io/sentry/SentryEvent.java 79.03% <0.00%> (+4.83%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae95ea3...e026dc3. Read the comment docs.

@@ -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);
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@maciejwalkowiak
Copy link
Contributor Author

@marandaneto take a look please if reading order makes sense. Currently first the Proguard UUID is read from the manifest, if not found from sentry-debug-meta.properties.

@@ -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);
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, missed that.

@maciejwalkowiak maciejwalkowiak merged commit dd6f9db into main Sep 20, 2021
@maciejwalkowiak maciejwalkowiak deleted the gh-1236 branch September 20, 2021 12:34
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.

Allow setting proguard via Options and/or external resources
3 participants