-
Notifications
You must be signed in to change notification settings - Fork 0
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
ID-822 Use new Sam endpoint for Signed URLs #77
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #77 +/- ##
============================================
- Coverage 84.17% 84.13% -0.04%
Complexity 228 228
============================================
Files 36 36
Lines 809 807 -2
Branches 75 75
============================================
- Hits 681 679 -2
Misses 96 96
Partials 32 32
☔ View full report in Codecov by Sentry. |
Integration test is expected to fail until broadinstitute/sam#1193 ships |
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.
Same comment about the UI PR: I have my doubts about the API
post: | ||
summary: gets a signed url for a blob using the pet service account for the user in the provided project | ||
summary: gets a signed url for a blob using the arbitrary pet service account for the user and an optional requester pays google project |
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.
Again, I'd love to understand why it needs to be arbitrary. The project should get passed in by the client. I worry this will cause permissions issues
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.
Removed all references to Pet Service Accounts
@@ -95,7 +95,7 @@ public static void setupDrsResolutionServiceMocks( | |||
any(BearerToken.class), | |||
eq(true), | |||
nullable(String.class), | |||
nullable(String.class)); | |||
eq(googleProject)); |
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.
I'm like 90% sure this is causing the tests to fail
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.
It totes was.
Kudos, SonarCloud Quality Gate passed! |
A new Signed URL Endpoint is being created in Sam to correctly handle signed urls for requester-pays buckets. DRSHub should use it.
broadinstitute/sam#1193