-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[DRAFT] feat(uploadFiles): add endpoint to get presigned upload url #14943
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
base: master
Are you sure you want to change the base?
[DRAFT] feat(uploadFiles): add endpoint to get presigned upload url #14943
Conversation
3d2e071
to
629229b
Compare
Bundle ReportChanges will increase total bundle size by 38 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: datahub-react-web-esmAssets Changed:
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
new HashSet<>( | ||
Arrays.asList( | ||
"pdf", "jpeg", "jpg", "png", "pptx", "docx", "xls", "xml", "ppt", "gif", "xlsx", | ||
"bmp", "doc", "rtf", "gz", "zip", "mp4", "mp3", "wmv", "tiff", "txt", "md", "csv")); |
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.
comma separated config in application.yaml
would make it easier to change this for anyone
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.
Thanks, I created a followup issue for that - https://linear.app/acryl-data/issue/CH-844/move-list-of-available-file-extensions-to-applicationyml
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.
awesome stuff so far
if (scenario == UploadDownloadScenario.ASSET_DOCUMENTATION) { | ||
return String.format("%s/product-assets/%s", bucketName, fileId); | ||
} else { | ||
throw new IllegalArgumentException("Unsupported upload scenario: " + scenario); | ||
} |
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.
yeah and we'll come back to this later in order to support other scenarios like home page modules and the like.
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 not positive we need to prefix with the bucket name here, but with local stack i'm seeing that's needed. it may be something we need to update once we deploy on a real instance with a real connection to s3
ASSET_DOCUMENTATION | ||
} | ||
|
||
type GetPresignedUploadUrl { |
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.
could rename to GetPresignedUploadUrlResponse
private void configureFilesResolver(final RuntimeWiring.Builder builder) { | ||
builder.type( | ||
"GetPresignedUploadUrlResponse", | ||
typeWiring -> | ||
typeWiring | ||
.dataFetcher( | ||
"latestVersion", | ||
new EntityTypeResolver( | ||
entityTypes, (env) -> ((VersionSet) env.getSource()).getLatestVersion())) | ||
.dataFetcher( | ||
"versionsSearch", | ||
new VersionsSearchResolver(this.entityClient, this.viewService))); | ||
} |
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 think this isn't necessary right?
this(s3Client, entityClient, null); | ||
} | ||
|
||
public S3Util( |
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.
let's double check @anshbansal has reviewed this file since he was the original implementer
* @param expirationSeconds The expiration time in seconds | ||
* @return The pre-signed URL | ||
*/ | ||
public String generatePresignedDownloadUrl( |
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.
okay nice so you brought this over from my WIP branch
'quickstartDebugAws': [ | ||
profile: 'debug-aws', | ||
modules: python_services_modules + backend_profile_modules + [':datahub-frontend', ':datahub-actions'], | ||
isDebug: true, | ||
additionalEnv: [ | ||
DATAHUB_LOCAL_ACTIONS_ENV: "${rootProject.project(':smoke-test').projectDir}/test_resources/actions/actions.env" | ||
] | ||
], |
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.
we want to keep this right? or not? i know you added it yourself for local dev purposes obviously
// AWS SDK for S3 file serving | ||
implementation platform('software.amazon.awssdk:bom:2.23.6') | ||
implementation 'software.amazon.awssdk:regions' | ||
implementation 'software.amazon.awssdk:sts' | ||
implementation 'software.amazon.awssdk:s3' |
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.
we will want to double check with at least one platform person whether this makes sense. i believe this was added for the S2UtilsFactory and we need that factory so we can us S3Utils in our new REST endpoint that will return a redirect to a download presigned url given a file ID. that will be in another PR.
This PR includes: