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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exclude spark from core jar #281

Merged
merged 14 commits into from Apr 19, 2023
Merged

Exclude spark from core jar #281

merged 14 commits into from Apr 19, 2023

Conversation

eneskuluk
Copy link
Contributor

Excludes spark files by executing maven-jar plugin once more to create a jar without spark. After maven-shade creates uber jar, replacing the excluded one with regular jar.

@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Merging #281 (22f65ca) into develop (ed40e6c) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##             develop     #281      +/-   ##
=============================================
+ Coverage      78.38%   78.43%   +0.05%     
  Complexity         1        1              
=============================================
  Files            141      141              
  Lines          22480    22480              
  Branches         408      408              
=============================================
+ Hits           17620    17633      +13     
+ Misses          4639     4626      -13     
  Partials         221      221              
Impacted Files Coverage Δ
.../genomicsdb/spark/api/GenomicsDBSparkBindings.java 93.33% <100.00%> (ø)

... and 8 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@eneskuluk
Copy link
Contributor Author

Currently it doesn't include the changes in mvnrepository artifact upload, we haven't talked that yet which was next step, that's why it is currently draft.

@eneskuluk eneskuluk changed the title Ek exclude spark from core jar Exclude spark from core jar Mar 28, 2023
Copy link
Member

@nalinigans nalinigans left a comment

Choose a reason for hiding this comment

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

Looks good so far @eneskuluk.

@eneskuluk
Copy link
Contributor Author

Added check for gatk in release test by checking skip-gatk-it in tag message. If skip-gatk-it present in tag message, It will skip gatk part, if not it will try to run gatk part. I saw that gatk uses java 17 as default in the test, until java 17 merges on our end, we should skip gatk part always by adding skip-gatk-it to the tag message. If not it will fail right away because it is asking for java 17.

@eneskuluk eneskuluk marked this pull request as ready for review April 15, 2023 12:03
if [[ ${VERSION_NUMBER} != *SNAPSHOT ]]; then
mvn nexus-staging:rc-list -DnexusUrl=https://oss.sonatype.org/ -DserverId=ossrh -f genomicsdb-${VERSION_NUMBER}.pom
stagingRepoId=$(mvn nexus-staging:rc-list -DnexusUrl=https://oss.sonatype.org/ -DserverId=ossrh | grep orggenomicsdb|cut -f2 -d' ')
echo $stagingRepoId
mvn nexus-staging:rc-close -DserverId=ossrh -DnexusUrl=https://oss.sonatype.org/ -DstagingRepositoryId=$stagingRepoId -f genomicsdb-${VERSION_NUMBER}.pom
mvn nexus-staging:rc-close -DserverId=ossrh -DnexusUrl=https://oss.sonatype.org/ -DstagingRepositoryId=$stagingRepoId -f genomicsdb-${VERSION_NUMBER}.pom
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be genomicsdb-spark-${VERSION_NUMBER}.pom based on what I see in the staging repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, that's correct. Missed that one somehow. Gonna fix it.

Copy link
Contributor

@mlathara mlathara left a comment

Choose a reason for hiding this comment

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

I think this mostly looks good -- one small comment/potential change

@@ -62,10 +70,11 @@ jobs:
dylib_artifact: libtiledbgenomicsdb.dylib.${{ github.ref_name }}

test:
needs: [release-jar]
needs: [build-and-push-mac-dylib, release-jar]
Copy link
Member

Choose a reason for hiding this comment

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

I have not understood how this TAG_MESSAGE is being used. Shouldn't release-jar already be dependent on build-and-push-mac-dylib? Do we have to explicitly call out the dependency here?

Copy link
Contributor Author

@eneskuluk eneskuluk Apr 18, 2023

Choose a reason for hiding this comment

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

Tag message is used for skipping or not skipping the gatk tests which @mlathara asked me to add. Dependency explicitly added so we can use output from that step to pass tag message to test step.

@eneskuluk eneskuluk requested a review from mlathara April 18, 2023 20:08
Copy link
Contributor

@mlathara mlathara left a comment

Choose a reason for hiding this comment

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

lgtm

@nalinigans nalinigans self-requested a review April 19, 2023 00:01
Copy link
Member

@nalinigans nalinigans left a comment

Choose a reason for hiding this comment

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

LGTM

@nalinigans nalinigans merged commit b0f4487 into develop Apr 19, 2023
22 checks passed
@nalinigans nalinigans deleted the ek_exclude_spark branch April 19, 2023 00:04
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.

None yet

3 participants