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.

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

Upload folder to cloud storage #38

Merged
merged 17 commits into from
Jan 4, 2023

Conversation

Shofiya2003
Copy link
Collaborator

@Shofiya2003 Shofiya2003 commented Jan 2, 2023

  • Created a common interface with a method 'Upload', which is implemented by structures of every cloud storage.
  • The individual structures contain constants like 'bucketname', 'containername' required to upload on that cloud storage. The logic of each cloud storage has been implemented by 'Upload' method on the individual structs.
  • This would make addition of new cloud storage efficient in the future.

Uploading to GCP

demo
success message

Uploading to AWS

demo
success message

Uploading to Azure

demo and success message

Closes #7

utils/upload.go Outdated Show resolved Hide resolved
utils/upload.go Show resolved Hide resolved
utils/upload.go Outdated Show resolved Hide resolved
utils/upload.go Outdated Show resolved Hide resolved
@uzaxirr
Copy link
Member

uzaxirr commented Jan 3, 2023

Hey Thanks for the PR,
Can you also add some screen shots depicting the working so that the review becomes easy

@Shofiya2003
Copy link
Collaborator Author

screenshots should include the success message, since I am using the functions to upload the folders. Right? @uzaxirr

@uzaxirr
Copy link
Member

uzaxirr commented Jan 3, 2023

screenshots should include the success message, since I am using the functions to upload the folders. Right? @uzaxirr

Absolutely correct, I'll suggest to create a new file for demo purpose, import the functions and run them. The output should be displayed in screenshot

@Shofiya2003
Copy link
Collaborator Author

@uzaxirr
added the screenshots and made the changes according to the review. suggest changes if any.
the workflow check is failing will look into it

@uzaxirr
Copy link
Member

uzaxirr commented Jan 3, 2023

@uzaxirr added the screenshots and made the changes according to the review. suggest changes if any. the workflow check is failing will look into it

Hey can you update the branch as well?

@Shofiya2003
Copy link
Collaborator Author

@uzaxirr added the screenshots and made the changes according to the review. suggest changes if any. the workflow check is failing will look into it

Hey can you update the branch as well?

done

Copy link
Member

@uzaxirr uzaxirr left a comment

Choose a reason for hiding this comment

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

Thanks for your valuable contribution. Here's a small suggestion
The log.Fatalln() is similar to log.Println(), but it also calls os.Exit(1) after logging the message. This causes the program to terminate with a non-zero exit code, indicating that an error occurred.
So It is generally not recommended to call log.Fatalln() or any other function that causes the program to terminate in functions other than main() or init().

Thus you can replace all the occurrence of log.Fatalln() with log.Errorln() or any other error level log

@@ -14,3 +14,7 @@ const S3_REGION = "us-east-1"
const AWS_ENDPOINT = "http://localhost:4566"
const PRESIGNED_URL_EXPIRATION = 60 * time.Minute
const OUTPUT_FILE_PATH_PREFIX = "output"
const GCP_BUCKET_NAME = ""
Copy link
Member

Choose a reason for hiding this comment

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

make it const GCP_BUCKET_NAME = "zstream-bucket"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

alright

utils/upload.go Show resolved Hide resolved
@Shofiya2003
Copy link
Collaborator Author

log does not have a method for error level log. Can I use logrus (https://github.com/Sirupsen/logrus) for the same?
@uzaxirr

@uzaxirr
Copy link
Member

uzaxirr commented Jan 3, 2023

log does not have a method for error level log. Can I use logrus (Sirupsen/logrus) for the same? @uzaxirr

Yeah, Even i was investigating it, you can use log.Println() for now, I'll open up a new issue for setting up a new logger across the repo.

@Shofiya2003
Copy link
Collaborator Author

will make sure the tests passes by tomorrow. Thanks a ton for help @uzaxirr

@Shofiya2003
Copy link
Collaborator Author

@uzaxirr
I was using defer with Close method of os.file , this led to issue with the checks.
used https://www.joeshaw.org/dont-defer-close-on-writable-files/ to solve the issues.
do suggest changes if any

@abskrj
Copy link
Member

abskrj commented Jan 4, 2023

@Shofiya2003 anything left to add in this PR?

@Shofiya2003
Copy link
Collaborator Author

no. @abhishekraj272
made the changes according to the review.

@abskrj
Copy link
Member

abskrj commented Jan 4, 2023

thanks @Shofiya2003

Copy link
Member

@abskrj abskrj left a comment

Choose a reason for hiding this comment

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

lgtm

@abskrj abskrj merged commit 64afcdf into ZeStream:dev Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upload function to upload whole folder to cloud storage
3 participants