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.

Sign up for GitHub

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

Jump to bottom

Add support for bugsnagReporting in gradle config. #820

Merged
merged 9 commits into from
Jul 30, 2023
Merged

Add support for bugsnagReporting in gradle config. #820

merged 9 commits into from
Jul 30, 2023

Conversation

tabletenniser
Copy link
Contributor

@tabletenniser tabletenniser commented Jul 25, 2023 •
edited

See #818. This PR essentially adds feature parity between analyticsTracking and bugsnagReporting

This PR also:

  • Fix a bug where we still track analytics even if analyticsTracking is set to False in the gradle config
  • Fix an incorrect CLI helper message on analyticsTracking where the default value is now to track analytics

Things are a bit messy today because analyticsTracking can be set in both Marathonfile / gradle config and CLI today. In the case where we run Marathon from gradle, we can only set it inside gradle config but not the CLI. Before this PR, analyticsTracking defaults to False for Marathon config but defaults to True for CLI. This makes it impossible to disable analyticsTracking when running Marathon from Gradle because the CLI's default True value will always take priority and overwrite whatever value we have in the Marathonfile / gradle config. This PR changes it such that we default to True for both Marathonfile / gradle config and CLI, and if we set it to False explicitly in either, we will disable the analytics tracking. A similar pattern is also applied to bugsnagReporting.

This PR is tested by:

  • End to end using the samples/android-app sample app where we see Noop exceptions reporter started with bugsnagReporting set to False in the gradle config.
  • Unit tests on ConfigurationFactory.kt

@tabletenniser tabletenniser mentioned this pull request Jul 25, 2023
Closed
Malinskiy
Malinskiy requested changes Jul 25, 2023
View reviewed changes
cli/src/main/kotlin/com/malinskiy/marathon/cli/ApplicationView.kt Show resolved Hide resolved
configuration/src/main/kotlin/com/malinskiy/marathon/config/Configuration.kt Show resolved Hide resolved
core/src/main/kotlin/com/malinskiy/marathon/Marathon.kt Outdated
@@ -181,8 +181,6 @@ class Marathon(
}

private fun logSystemInformation() {
log.info { "System Information:" }
Copy link
Member

Choose a reason for hiding this comment

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

How is this relevant to the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, that was not intentional, added it back.

marathon-gradle-plugin/src/main/kotlin/com/malinskiy/marathon/gradle/MarathonExtension.kt
/**
* Whether to report crashes to Bugsnag. By default, this is enabled. Use **false** to disable.
*/
var bugsnagReporting: Boolean = true
Copy link
Member

Choose a reason for hiding this comment

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

👍

sample/android-app/app/build.gradle.kts Outdated
@@ -51,6 +51,8 @@ android {
}

marathon {
analyticsTracking = false
Copy link
Member

Choose a reason for hiding this comment

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

Please remove these from the sample app. Instead, maybe the docs need an example of disabling bugsnag for both CLI and for Gradle, currently there are none.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed and updated docs/docs/intro/configure.md

...guration/src/main/kotlin/com/malinskiy/marathon/config/serialization/ConfigurationFactory.kt Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 25, 2023 •
edited

Codecov Report

Merging #820 (e1dd916) into develop (1f4905b) will increase coverage by 0.04%.
The diff coverage is 77.77%.

❗ Current head e1dd916 differs from pull request most recent head 72b04d6. Consider uploading reports for the commit 72b04d6 to get more accurate results

@@              Coverage Diff              @@
##             develop     #820      +/-   ##
=============================================
+ Coverage      59.55%   59.59%   +0.04%     
- Complexity       794      799       +5     
=============================================
  Files            213      213              
  Lines           4374     4381       +7     
  Branches         688      690       +2     
=============================================
+ Hits            2605     2611       +6     
- Misses          1444     1445       +1     
  Partials         325      325
Files Changed Coverage Δ
...lin/com/malinskiy/marathon/config/Configuration.kt 51.21% <66.66%> (+0.27%) ⬆️
...athon/config/serialization/ConfigurationFactory.kt 83.56% <100.00%> (+1.87%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Malinskiy
Malinskiy reviewed Jul 26, 2023
View reviewed changes
docs/docs/intro/configure.md Outdated
@@ -343,6 +344,42 @@ marathon {
</TabItem>
</Tabs>

NOTE: analyticsTracking can also be enabled (default value) / disabled directly from the CLI. It is disabled if it's set to be disabled in either the config or the CLI.
Copy link
Member

Choose a reason for hiding this comment

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

Can you please just use https://docusaurus.io/docs/markdown-features/admonitions?
There are examples in the codebase for them already

Malinskiy
Malinskiy requested changes Jul 26, 2023
View reviewed changes
docs/docs/intro/configure.md Outdated
</TabItem>
</Tabs>

NOTE: bugsnagReporting can also be enabled (default value) / disabled directly from the CLI. It is disabled if it's set to be disabled in either the config or the CLI.
Copy link
Member

Choose a reason for hiding this comment

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

Same with admonitions

Malinskiy
Malinskiy previously approved these changes Jul 27, 2023
View reviewed changes
@tabletenniser
Copy link
Contributor Author

Thanks @Malinskiy for the review! Are you going to merge this PR (since I obviously don't have permission to) or do we still need reviews from @tagantroy and @Vacxe ?

@Malinskiy
Copy link
Member

I'm waiting for one more lgtm for a day and will merge afterwards assuming no issues.

matzuk
matzuk previously approved these changes Jul 28, 2023
View reviewed changes
@tabletenniser tabletenniser dismissed stale reviews from matzuk and Malinskiy via f8444e1 July 28, 2023 06:27
@matzuk matzuk self-requested a review July 29, 2023 13:21
@matzuk
Copy link
Collaborator

matzuk commented Jul 29, 2023

@Malinskiy let's merge

@Malinskiy Malinskiy merged commit 0354fc9 into MarathonLabs:develop Jul 30, 2023
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matzuk matzuk matzuk approved these changes

@Malinskiy Malinskiy Malinskiy left review comments

@tagantroy tagantroy Awaiting requested review from tagantroy tagantroy is a code owner

@Vacxe Vacxe Awaiting requested review from Vacxe Vacxe is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tabletenniser @Malinskiy @matzuk