Skip to content

Conversation

@maciejwalkowiak
Copy link
Contributor

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Remove duplicated Novoda plugin config.

Configuration has been moved from each module to parent build.gradle.kts into subprojects section.

💡 Motivation and Context

getsentry/sentry-android#537 (comment)

💚 How did you test it?

Build distribution packages distZip and compared with distributions created before the change.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing
  • No breaking changes

🔮 Next steps

@marandaneto since I am not very fluent in Gradle I would appreciate your deeper review here.

build.gradle.kts Outdated
publishVersion = project.version.toString()
desc = Config.Sentry.description
website = Config.Sentry.website
repoName = Config.Sentry.javaRepoName
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok now I see the difference between Java and Android repo.

Copy link
Member

Choose a reason for hiding this comment

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

What is the difference? We moved from https://github.com/getsentry/sentry-android into this one, isnt' it all the same now?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, is this a bintray repo?

Copy link
Member

Choose a reason for hiding this comment

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

if so could it be named bintrayRepo please?

Copy link
Contributor

Choose a reason for hiding this comment

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

no, java packages go under sentry-java, android packages go sentry-android and its not possible to change, we'd lose jcenter sync cus its a bintray limitation

Copy link
Member

Choose a reason for hiding this comment

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

yeah I realize that on the second comment.
I suggested rename the variable. repoName is very ambiguous

@marandaneto
Copy link
Contributor

@maciejwalkowiak I found problems doing something similar but on a few older Gradle's version , the kotlin build scripts were a bit limited yet, I think it should be fine now.
Just run make dryRelease, and you should be able to see the output of the release correctly.

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

thanks for doing this

@maciejwalkowiak
Copy link
Contributor Author

Thanks @marandaneto. The output of make dryRelease looks good as far as i can tell.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

SO happy to see this!

build.gradle.kts Outdated
publishVersion = project.version.toString()
desc = Config.Sentry.description
website = Config.Sentry.website
repoName = if (project.name.contains("android")) Config.Sentry.androidRepoName else Config.Sentry.javaRepoName
Copy link
Member

Choose a reason for hiding this comment

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

The suggestion is this, though PR is good to merge:

Suggested change
repoName = if (project.name.contains("android")) Config.Sentry.androidRepoName else Config.Sentry.javaRepoName
repoName = if (project.name.contains("android")) Config.Sentry.androidBintrayRepoName else Config.Sentry.javaBintrayRepoName

Copy link
Contributor

Choose a reason for hiding this comment

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

it makes easier to read but also verbose, its setting in the bintray plugin repoName field so it's kinda implicit that its bintray's repo name, I'm fine with this change though.
If you had a doubt about the var you are probably right, I wrote this part myself :D

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@17eb856). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1007   +/-   ##
=======================================
  Coverage        ?   71.61%           
  Complexity      ?     1299           
=======================================
  Files           ?      134           
  Lines           ?     4731           
  Branches        ?      490           
=======================================
  Hits            ?     3388           
  Misses          ?     1087           
  Partials        ?      256           
Impacted Files Coverage Δ Complexity Δ
...in/java/io/sentry/transport/NoOpTransportGate.java 100.00% <0.00%> (ø) 3.00% <0.00%> (?%)
...y/src/main/java/io/sentry/InvalidDsnException.java 23.07% <0.00%> (ø) 1.00% <0.00%> (?%)
sentry/src/main/java/io/sentry/protocol/App.java 96.55% <0.00%> (ø) 18.00% <0.00%> (?%)
.../io/sentry/spring/SentryInitBeanPostProcessor.java 92.30% <0.00%> (ø) 5.00% <0.00%> (?%)
sentry/src/main/java/io/sentry/protocol/Gpu.java 100.00% <0.00%> (ø) 22.00% <0.00%> (?%)
...src/main/java/io/sentry/SentryExecutorService.java 100.00% <0.00%> (ø) 6.00% <0.00%> (?%)
...c/main/java/io/sentry/ShutdownHookIntegration.java 100.00% <0.00%> (ø) 4.00% <0.00%> (?%)
...ava/io/sentry/SendFireAndForgetEnvelopeSender.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
...main/java/io/sentry/protocol/SentryStackFrame.java 35.00% <0.00%> (ø) 14.00% <0.00%> (?%)
sentry/src/main/java/io/sentry/Hub.java 65.06% <0.00%> (ø) 59.00% <0.00%> (?%)
... and 124 more

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 17eb856...982ec7f. Read the comment docs.

@maciejwalkowiak maciejwalkowiak merged commit dd941d7 into main Oct 23, 2020
@maciejwalkowiak maciejwalkowiak deleted the build-gradle-cleanup branch October 23, 2020 08:38
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.

5 participants