-
Notifications
You must be signed in to change notification settings - Fork 371
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
Conversation
c4f937c
to
939f41b
Compare
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 |
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
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 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. |
Thank you @kshakir |
Description
is compiled with Java 19Checklist (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
Review
For more detailed guidelines, see https://github.com/broadinstitute/picard/wiki/Guidelines-for-pull-requests