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

Update google-cloud-nio to support underscores in bucket names #1903

Merged
merged 1 commit into from Jul 27, 2023

Conversation

kshakir
Copy link
Contributor

@kshakir kshakir commented Jul 27, 2023

Description

  • The latest version of the google-cloud-nio library now supports bucket names with underscores
  • The google-cloud-nio library is compatible with Java 7+ but is compiled with Java 19
    • EDIT: google-cloud-nio pulls in jackson-core 2.15.2
    • jackson-core 2.15.2 is a Multi-Release Jar that includes optional Java 19 classes
  • The shadowJar 7.x plugin doesn't work with Java 19 compiled classes, but 8.x does
  • shadowJar 8.x only works with gradle 8.x
  • upgrading gradle from 7.x to 8.x required a few changes to the build

Checklist (never delete this)

Never delete this, it is our record that procedure was followed. If you find that for whatever reason one of the checklist points doesn't apply to your PR, you can leave it unchecked but please add an explanation below.

Content

  • Added or modified tests to cover changes and any new functionality
  • Edited the README / documentation (if applicable)
  • All tests passing on github actions

Review

  • Final thumbs-up from reviewer
  • Rebase, squash and reword as applicable

For more detailed guidelines, see https://github.com/broadinstitute/picard/wiki/Guidelines-for-pull-requests

@kshakir
Copy link
Contributor Author

kshakir commented Jul 27, 2023

The current 939f41b is proof of at least one exit from Dependency Hell 🔥

There are probably other alternatives to fixing the chain of issues in the description. Let me know what you think, as either way we can use something similar over in broadinstitute/gatk#8439

Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

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

@kshakir 1 typo and 1 question. Good to merge after the the typo is fixed.

import java.nio.file.Path;
import java.nio.file.Paths;

public class GATKBucketUtilsTest {
Copy link
Member

Choose a reason for hiding this comment

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

Why move this tests here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied this here because I didn't see another test to catch if support for underscores in bucket names regressed.

I added the test after seeing "Added or modified tests to cover changes and any new functionality" in the checklist in the issues template.

Copy link
Member

Choose a reason for hiding this comment

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

Heh, right, that makes sense. I forgot why we were doing this in the first place...

build.gradle Outdated
}
}

wrapper {
gradleVersion = '7.5.1'
gradleVersion = '8.1.1'
Copy link
Member

Choose a reason for hiding this comment

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

There seems to be a mismatch here, this says 8.1.1 but the wrapper properties says 8.2.1. I think this should be 8.2.1.

@kshakir kshakir removed their assignment Jul 27, 2023
@kshakir kshakir requested a review from lbergelson July 27, 2023 15:39
@lbergelson
Copy link
Member

@kshakir Thanks for the clear explanation and the improvements to the gradle build. This sounds like one of those painful to track down explanations. I suspect if necessary we could avoid the need to upgrade by stripping the java 19 class files from the shadow jar instead of processing them. I think upgrading gradle/shadow is better though, and since you've just done it I don't see any reason to do it differently here... I'm hoping it's not too much harder to update the gatk gradle build to 8 as well?

This puts us in an easier thank position to update to java 21 in September too.

@lbergelson lbergelson merged commit 47094da into master Jul 27, 2023
6 checks passed
@lbergelson lbergelson deleted the ks_bump_gcs_nio branch July 27, 2023 17:18
@lbergelson
Copy link
Member

Thank you @kshakir

@kshakir kshakir mentioned this pull request Jul 28, 2023
5 tasks
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

2 participants