-
-
Notifications
You must be signed in to change notification settings - Fork 35
feat(size-analysis): Add upload functionality for size analysis #915
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
base: main
Are you sure you want to change the base?
Conversation
6e01215
to
00bd9a2
Compare
plugin-build/src/main/kotlin/io/sentry/android/gradle/AndroidComponentsConfig.kt
Outdated
Show resolved
Hide resolved
if (bundleFile != null) { | ||
if (bundleFile.exists()) { | ||
args.add(bundleFile.path) | ||
return |
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.
actually, if we need to add more arguments to the upload (like proguard or other files), this is perhaps not the best design since it relies on early returns.
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.
Yeah I think we might need to add mappings for apks, so I think a when statement would be better here, like when AAB, args are just aab, when APK we could append proguard mappings (in future when added to CLI), etc.
plugin-build/src/agp74/kotlin/io/sentry/android/gradle/AGP74Compat.kt
Outdated
Show resolved
Hide resolved
661868d
to
c6f323b
Compare
val enabled: Property<Boolean> = | ||
objects | ||
.property(Boolean::class.java) | ||
.convention(providerFactory.isCi() && false) // set to false for now otherwise upload fails CI |
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.
This is hardcoded to false for the time being so that we don’t auto-upload while it isn’t working.
c6f323b
to
9f85630
Compare
@@ -2,6 +2,10 @@ | |||
|
|||
## Unreleased | |||
|
|||
### Features | |||
|
|||
- Add upload functionality for size analysis ([#915](https://github.com/getsentry/sentry-android-gradle-plugin/pull/915)) |
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.
I guess we'd need to expand on this, like mention the new sizeAnalysis extension and when the builds are uploaded by default, but this can be done later when we GA this
|
||
@get:InputFile | ||
@get:Optional | ||
@get:PathSensitive(PathSensitivity.NAME_ONLY) |
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.
should this be RELATIVE? In case e.g. the variant changes (e.g. build/outputs/${variant}/aab
)? Same for the apk
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 the variant changes then the content of the file will change as well. I don't see why the location of file is important here though. It won't affect the directory in the uploaded zip.
): Pair<TaskProvider<SentryUploadAppArtifactTask>, TaskProvider<SentryUploadAppArtifactTask>> { | ||
val uploadMobileBundleTask = | ||
project.tasks.register( | ||
"uploadSentryBundle$taskSuffix", |
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.
"uploadSentryBundle$taskSuffix", | |
"uploadSentryBundle${taskSuffix}ForSizeAnalysis", |
don't know if we have to be specific here, but just a suggestion
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.
This is a good point, but we might do more than just size analysis with the upload.
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.
Would be cool to add an integration test - you can take a look at https://github.com/getsentry/sentry-android-gradle-plugin/blob/main/plugin-build/src/test/kotlin/io/sentry/android/gradle/integration/SentryPluginIntegrationTest.kt which tests uploads against a simple python server spawned locally
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.
i've added this! good suggestion!
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.
minor things, but nothing blocking. LGTM, great stuff!
# Conflicts: # examples/android-guardsquare-proguard/sentry.properties # plugin-build/sentry-cli.properties
9f85630
to
a7a245b
Compare
e653a47
to
44a8525
Compare
|
31744bb
to
03b3c23
Compare
51c8e48
to
b9751ad
Compare
d2f0f7f
to
ed2fa6d
Compare
@@ -1,2 +1,2 @@ | |||
version = 2.52.0 | |||
version = 2.53.0-alpha |
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.
This is so that the integration tests use 2.53.0-alpha and pass.
📜 Description
This adds a new task
SentryUploadAppArtifactTask
which calls tosentry-cli mobile-app upload (apk or abb).
Most sentry functionality is enabled by the variant filter in the sentry extension. However, the size upload functionality is enabled by default when we are on CI.
This is because some teams might want to enable debug builds to upload or have some different logic than the regular variant filter.
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
🔮 Next steps