-
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
presigned url 적용 #147
presigned url 적용 #147
Conversation
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.
귀찮고 헷갈리는 작업 많았을텐데 넘 잘해주셨네요~!! 👍
@@ -15,7 +15,7 @@ export class AppController { | |||
/** | |||
* 광고 이미지 응답 | |||
*/ | |||
@ApiTags('COMPLETE') | |||
@ApiTags('LEGACY') |
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.
LEGACY 태그로 기존 코드 구분하신게 좋은거 같아요!
* 특정 광고 이미지의 presigned url만 받고 싶은 경우 | ||
* @example 'test.webp' |
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.
👍
( | ||
await createPresignedUrl( | ||
process.env.INPUT_BUCKET, | ||
`${videoId}.${videoExtension}`, | ||
'PUT', | ||
) | ||
).url, |
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.
아래 코드는 어떠신가요?
createPresignedUrl(
process.env.INPUT_BUCKET,
`${videoId}.${videoExtension}`,
'PUT',
).then( data=> data.url)
server/src/ncpAPI/presignedURL.ts
Outdated
new HttpRequest({ ...url, method }), | ||
{ expiresIn: 100 }, | ||
); | ||
return { name: objectName, url: formatUrl(signedUrlObject) }; |
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.
url만 리턴하는게 자연스러울거 같은데 name을 같이 리턴해야만 하는 이유가 있을까요?
이 함수를 호출하는 코드에서도 url만 사용하는 경우가 있는 것 같아서요!
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.
광고 이미지를 재발급 받으려면 name이 필요한데 서비스에서 추가하기 귀찮아서 이렇게 했습니다..!
) | ||
).url | ||
: null; | ||
if (profileDto.profileExtension === '' && user.profileImageExtension) { |
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.
'profileImage' in profileDto 보다 직관적이네요 좋습니다
const result = ( | ||
await this.UserModel.findOneAndUpdate({ uuid }, updateOption) | ||
).toObject(); |
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.
아마 toObject()를 lean:true로 대체할 수 잇을거같아요~
server/src/video/video.service.ts
Outdated
videoInfo.thumbnailExtension | ||
? ( |
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 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 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.
리뷰 반영 감사합니다 🙇🏻♂️
resolved: #146
작업 내용