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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate Sentry BOM #1486

Merged
merged 16 commits into from
Jul 28, 2021
Merged

Generate Sentry BOM #1486

merged 16 commits into from
Jul 28, 2021

Conversation

maciejwalkowiak
Copy link
Contributor

馃摐 Description

Generate Sentry BOM

馃挕 Motivation and Context

Fixes #1484

馃挌 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

馃敭 Next steps

Update release.kt script BOM to publish to Maven central.

@maciejwalkowiak maciejwalkowiak marked this pull request as ready for review May 25, 2021 10:11
@maciejwalkowiak
Copy link
Contributor Author

Since I am not sure how to release the BOM file, the release.kt file will have to be updated likely when doing the release to find out if its enough to provide -Dfile property, or it has to be in -DpomFile etc.

@codecov-commenter
Copy link

codecov-commenter commented May 25, 2021

Codecov Report

Merging #1486 (e7eb11d) into main (cdc87ea) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #1486   +/-   ##
=========================================
  Coverage     75.93%   75.93%           
  Complexity     2004     2004           
=========================================
  Files           202      202           
  Lines          6943     6943           
  Branches        691      691           
=========================================
  Hits           5272     5272           
  Misses         1335     1335           
  Partials        336      336           

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 cdc87ea...e7eb11d. Read the comment docs.

@marandaneto
Copy link
Contributor

I published it locally and tried using mavenLocal but it does not work

A problem occurred configuring project ':app'.

Could not resolve all dependencies for configuration ':app:debugCompileClasspath'.
Could not find io.sentry:sentry-bom:5.0.0-beta.5-SNAPSHOT.
Required by:
project :app
Could not find io.sentry:sentry:.
Required by:
project :app

the other thing is, release.kts won't work as it's a single pom and does not contain any javadoc, sources file, so we'd need to special case this project in that script.
the folder distributions also does not contain the sentry-bom subfolder, I think it's not generating the zip file, this should also be somehow solved.

@marandaneto
Copy link
Contributor

just tried locally:

    implementation platform("io.sentry:sentry-bom:{version}")
    implementation "io.sentry:sentry-android"

and it worked, thanks @maciejwalkowiak

@marandaneto
Copy link
Contributor

@marandaneto
Copy link
Contributor

@maciejwalkowiak do you prefer opening a new PR for fixing the release script or this one?
it can be tested locally if temp. modifying settings.xml to maven local

@maciejwalkowiak
Copy link
Contributor Author

How the settings should be modified?

@marandaneto
Copy link
Contributor

How the settings should be modified?

ups sorry, release.kts, instead of using repositoryUrl as the cloud provider, set it to use maven local, so you can double check the deployment in ~/.m2/repository

@maciejwalkowiak
Copy link
Contributor Author

@marandaneto updated the release script. Works well when I tested it locally.


val file: String

val androidFile = folder
.listFiles { it -> it.name.contains("release") && it.extension == "aar" }
.firstOrNull()

if (androidFile != null) {
file = androidFile.path
val bomFile = folder
Copy link
Contributor

Choose a reason for hiding this comment

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

@iker-barriocanal these changes will need to be updated in the craft release too, right?

CHANGELOG.md Outdated Show resolved Hide resolved
@maciejwalkowiak
Copy link
Contributor Author

@marandaneto should we merge it or not yet?

@marandaneto
Copy link
Contributor

@marandaneto should we merge it or not yet?

I will test later but also need to check out #1486 (comment)
otherwise, the Draft of Craft will be broken already

@marandaneto
Copy link
Contributor

@iker-barriocanal I am about to merge this, is craft ready to do a bom publish too?

Comment on lines 65 to 70
val command = "./mvnw gpg:sign-and-deploy-file " +
"-Dfile=$pomFile " +
"-DpomFile=$pomFile " +
"-DrepositoryId=$repositoryId " +
"-Durl=$repositoryUrl " +
"--settings $settingsPath"
Copy link
Contributor

Choose a reason for hiding this comment

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

@iker-barriocanal how craft will handle this, as it's a different way of calling maven CLI for a bom (which is only a pom.xml) file?

@bruno-garcia
Copy link
Member

bruno-garcia commented Jul 21, 2021

An update, I merged the docs (getsentry/sentry-docs#3832) which I assume I'll need to revert. But to let y'all know we're just blocked on being able to release this. Hopefully @iker-barriocanal can help us out on this one too

@iker-barriocanal
Copy link
Member

@bruno-garcia @marandaneto opened a PR to support this in Craft: getsentry/craft#270.

@bruno-garcia
Copy link
Member

@bruno-garcia @marandaneto opened a PR to support this in Craft: getsentry/craft#270.

And it's merged! thanks Iker, I guess we're close to merging this one then

@marandaneto marandaneto merged commit 754583c into main Jul 28, 2021
@marandaneto marandaneto deleted the gh-1484 branch July 28, 2021 12:28
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.

Support Sentry BOM
5 participants