-
Notifications
You must be signed in to change notification settings - Fork 134
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
Integrate Tracks Gradle plugin #5214
Conversation
You can test the changes on this Pull Request by downloading the APK here. |
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 so much for this great improvement @wzieba! I've tested all the scenarios and it works as expected. I'm not merging to give @oguzkocer a chance to review it.
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 code changes look good, I left a comment about the tracksEnabled
flag in the gradle.properties-example
file.
I didn't test this extensively (following the instructions) as I assume @malinajirka already did. However, I did play with it a bit - especially the state where the tracking is not enabled in gradle.properties
- and I found that we print out the below message for ./gradlew -q
command:
✅ Build time report of 0m 0s has been received by Tracks.
There are 2 things that are a bit bothersome for me:
- When the tracking is off, the plugin shouldn't do anything and should be completely disabled. The messaging here suggests that the build time was tracked. (it shouldn't be tracked nor should it print anything)
- This is not as important, but it looks like this is printed with a
println
statement in the plugin, instead of using the logger. In my opinion, these logs shouldn't show up when we are running the commands with the-q
command. I don't think it's a blocker for this PR (I think the 1st point is) but it'd be great to handle it sooner rather than later.
I think it's also worth pinging the rest of @woocommerce/owl-team in case anyone else from the team would like to check it out before it's merged into develop
.
gradle.properties-example
Outdated
@@ -44,3 +44,7 @@ wc.in_app_update_type = 0 | |||
|
|||
# Necessary for gradle 5 | |||
kapt.incremental.apt=false | |||
|
|||
# Measuring build times | |||
tracksEnabled = true |
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 think we should comment this out by default as well. Our instructions for contributors copies this file as is, and unless we specifically tell them about the tracking in the instructions, it feels wrong to track their build times.
Good catch! It works as expected, when |
Thanks @oguzkocer @malinajirka ! Answering your concernes:
I wanted to make tracking as opt-out because by default I thought it's good idea to track all of the builds and don't make developers to remember about setting a flag to I agree that it feel wrong for doing this for external contributors though. So I'll make this opt-in for external contributors, but for our developers
I agree with you and that's one of the feedbacks you gave lately, in pair with not collecting build times if they are not meant to be sent. I've just created issues to fix both of them. As those improvements to the Gradle plugin are quite small, I'll mark this PR as draft for now, fix the plugin and this PR and re-request your reviews @oguzkocer @malinajirka
|
build.gradle
Outdated
tracks { | ||
automatticProject.set(TracksExtension.AutomatticProject.WooCommerce) | ||
username.set(findProperty('tracksUsername')) | ||
uploadEnabled.set((findProperty('tracksEnabled') ?: true).toBoolean()) |
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.
It's findProperty
and not getProperty
because I didn't want to cause build crash for developers that won't update gradle.properties
immediately after this PR. Does it sound good or should we rather enforce gradle.properties
update?
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 think that's the right decision since these properties are optional.
I've applied improvements in If it looks good to you, please don't merge this PR yet as just after this PR will be merged, I'll update internal
which will make measuring enabled by default for our developers. Does it sound good to you? |
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.
@wzieba Thank you for the improvements. I've tested the tracksEnabled = false
and there were no extra logs as expected. The scan also showed only 0.008s
of setup time for the plugin which is great!
Sorry, I was afk. Thanks for the review @oguzkocer and thanks for the improvements @wzieba ! 👏 |
Closes: #5212
Description
This PR integrates Tracks-Gradle plugin.
Please note that accepting this PR is also accepting change to secrets
gradle.properties
with same values as ingradle.properties-example
in this PR.Testing instructions
Before starting tests, please set debug mode for easier testing:
debug
flag totracks
cp gradle.properties-example gradle.properties
./gradlew tasks
"username":null
in payloaddebug
flag totracks
tracksUsername = test
togradle.properties
./gradlew tasks
"username":"test"
in payloadtracksEnabled = false
togradle.properties
./gradlew tasks
Images/gif
RELEASE-NOTES.txt
if necessary.