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.

Sign up for GitHub

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

Jump to bottom

Image uploader: Fix uploading of images to GCS #26493

Merged
merged 22 commits into from
Oct 24, 2020
Merged

Image uploader: Fix uploading of images to GCS #26493

merged 22 commits into from
Oct 24, 2020

Conversation

gastonqiu
Copy link
Contributor

@gastonqiu gastonqiu commented Jul 21, 2020 •
edited by aknuds1
Loading

What this PR does / why we need it:
Upload image to GCS with SDK to simplify the code. And make it easier to maintain.

Which issue(s) this PR fixes:
The GCS upload URL expires because the user can't upload the image file to GCS.

Fixes #26194

Special notes for your reviewer:

@gastonqiu gastonqiu requested a review from a team as a code owner July 21, 2020 17:45
@gastonqiu gastonqiu requested review from aknuds1 and jessabe and removed request for a team July 21, 2020 17:45
@aknuds1 aknuds1 added the pr/external This PR is from external contributor label Jul 22, 2020
aknuds1
aknuds1 reviewed Jul 22, 2020
View reviewed changes
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

I will have to test your PR before I can conclude on anything, but left some suggestions for improvements.

pkg/components/imguploader/gcsuploader.go Outdated Show resolved Hide resolved
pkg/components/imguploader/gcsuploader.go Outdated Show resolved Hide resolved
pkg/components/imguploader/gcsuploader.go Outdated Show resolved Hide resolved
pkg/components/imguploader/gcsuploader.go Outdated Show resolved Hide resolved
pkg/components/imguploader/gcsuploader.go Outdated Show resolved Hide resolved
@gastonqiu gastonqiu requested a review from aknuds1 July 23, 2020 07:31
@stale
Copy link

stale bot commented Aug 6, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale Issue with no recent activity label Aug 6, 2020
@aknuds1 aknuds1 removed the stale Issue with no recent activity label Aug 6, 2020
@aknuds1
Copy link
Contributor

aknuds1 commented Aug 6, 2020

Sorry, quite busy now, still have it on my agenda to test/review this.

@gastonqiu
Copy link
Contributor Author

It's OK. Take your time.

@stale
Copy link

stale bot commented Aug 22, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale Issue with no recent activity label Aug 22, 2020
@aknuds1 aknuds1 removed the stale Issue with no recent activity label Aug 23, 2020
@stale
Copy link

stale bot commented Sep 6, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale Issue with no recent activity label Sep 6, 2020
@gastonqiu
Copy link
Contributor Author

@aknuds1 This has been marked as stale. Can you help to remove the tag? Also does it help if I test this by myself and post the result here?

@stale stale bot removed the stale Issue with no recent activity label Sep 8, 2020
@aknuds1
Copy link
Contributor

aknuds1 commented Sep 8, 2020

The stale bot applies the label automatically. I haven't forgot about the PR, but I'm very busy with a number of different PRs plus own tasks to take care of.

gaston.qiu added 3 commits September 19, 2020 12:41
Chore: Fixed gcs uploader error due to url expired
65aa1b6 
Upload image to gcs with gcp sdk to simplify the code. And make it
more easy to maintain.
chore: rename uploadFileWithSDK to uploadFile
4742fa9
Chore: Modify log messages
bc0b3ba 
- Modify log message according to review grafana#26493 to make it more clear.
@stale
Copy link

stale bot commented Oct 3, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Fix: The signed url should not be removed
a2d80b5 
- The signed url function had been removed accidentally after rebase
to master so I add the function back.

grafana#26493 (comment)

Signed-off-by: gaston.qiu <gaston.qiu@umbocv.com>
aknuds1 and others added 2 commits October 17, 2020 11:08
@aknuds1
Merge branch 'chore/gcsUploadErr' of github.com:gastonqiu/grafana int…
8466347 
…o chore/gcsUploadErr

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Fix: The signed url should not be removed
039b3b8 
- The signed url function had been removed accidentally after rebase
to master so I add the function back.

grafana#26493 (comment)

Signed-off-by: gaston.qiu <gaston.qiu@umbocv.com>
@aknuds1
Copy link
Contributor

aknuds1 commented Oct 23, 2020

Sorry for the long wait, but I'm testing the PR now.

@aknuds1
Fix uploading to GCS
6b5ee65 
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1
Upgrade Go Storage SDK
5b277cf 
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
aknuds1
aknuds1 approved these changes Oct 23, 2020
View reviewed changes
Copy link
Contributor

@aknuds1 aknuds1 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! Tested and works well both with and without signed URLs.

Please note that I had to rewrite a bit to make it function.

@aknuds1 aknuds1 requested a review from papagian October 23, 2020 16:10
@aknuds1 aknuds1 added this to the 7.4 milestone Oct 23, 2020
@aknuds1 aknuds1 changed the title Chore: Fixed gcs uploader error due to url expired Image uploader: Fix uploading of images to GCS Oct 23, 2020
@aknuds1
Clean up
7dacbca 
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1
Clean up
10478cc 
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1 aknuds1 merged commit 1646de4 into grafana:master Oct 24, 2020
@aknuds1
Copy link
Contributor

aknuds1 commented Oct 24, 2020

Thanks for contributing!

aknuds1 added a commit that referenced this pull request Nov 3, 2020
@gastonqiu @aknuds1
Image uploader: Fix uploading of images to GCS (#26493)
93871af 
* GCS image uploader: Re-implement with Google SDK

Signed-off-by: gaston.qiu <gaston.qiu@umbocv.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
ryantxu pushed a commit that referenced this pull request Nov 18, 2020
@gastonqiu @aknuds1 @ryantxu
Image uploader: Fix uploading of images to GCS (#26493)
b815b43 
* GCS image uploader: Re-implement with Google SDK

Signed-off-by: gaston.qiu <gaston.qiu@umbocv.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
@mjseaman mjseaman added this to the 7.4.0 milestone Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aknuds1 aknuds1 aknuds1 approved these changes

@kylebrandt kylebrandt kylebrandt approved these changes

@jessabe jessabe Awaiting requested review from jessabe

@papagian papagian Awaiting requested review from papagian

Assignees
No one assigned
Labels
add to changelog area/backend area/image-rendering area/imagestore pr/external This PR is from external contributor type/bug
Projects
None yet
Milestone
7.4.0-beta1
Development

Successfully merging this pull request may close these issues.

GCS external image upload error response status code 404
5 participants
@gastonqiu @aknuds1 @kylebrandt @mjseaman @jessabe