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

Added Endpoint for pre signing url for azure/gcp #64

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

abskrj
Copy link
Member

@abskrj abskrj commented Jan 16, 2023

Fixes #63

@abskrj abskrj marked this pull request as ready for review January 16, 2023 17:15
@abskrj abskrj requested review from Shofiya2003 and uzaxirr January 16, 2023 17:15
containerName := constants.CLOUD_CONTAINER_NAME_TEMP

if configs.EnvVar[configs.GCP_PROJECT_ID] != "" {
return getPresignedGCPURL(&GcpUploader{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a suggestion, correct me if I am wrong .why are we passing a pointer to the function generating predesigned URL for GCP, shouldn't the code be uniform for every url generating function?

@@ -221,5 +288,15 @@ func (a AzureUploader) Upload(walker fileWalk) {
log.Println("Unable to close the file ", path)
}
}
}

func getPresignedAzureURL(a AzureUploader) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we need to add expiry time for the azure presigned URL

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.

will cloudSession := configs.GetCloudSession() return an error if session is not able to be initialized?. If yes then should add a default case to the switch statement to handle this scenario?

also the function is not returning any error, if the presigned URL generation fails. It would be better to return an error along with the presigned URL so that the func caller can handle the error.

Comment on lines +83 to +102
if configs.EnvVar[configs.GCP_PROJECT_ID] != "" {
return getPresignedGCPURL(&GcpUploader{
ContainerName: containerName,
VideoId: videoId,
Client: cloudSession.GCPSession,
})
}

if configs.EnvVar[configs.AWS_ACCESS_KEY_ID] != "" {
return getPresignedAWSURL(AwsUploader{
ContainerName: containerName,
VideoId: videoId,
Session: cloudSession.AWSSession,
})
}

if configs.EnvVar[configs.AZURE_ACCESS_KEY] != "" {
return getPresignedAzureURL(AzureUploader{
ContainerName: containerName,
VideoId: videoId,
Copy link
Member

Choose a reason for hiding this comment

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

Use switch-case whenever multiple if-else are involved

switch configs.EnvVar[configs.GCP_PROJECT_ID] {
case "":
    return getPresignedGCPURL(&GcpUploader{
        ContainerName: containerName,
        VideoId:       videoId,
        Client:        cloudSession.GCPSession,
    })
case "":
    return getPresignedAWSURL(AwsUploader{
        ContainerName: containerName,
        VideoId:       videoId,
        Session:       cloudSession.AWSSession,
    })
case "":
    return getPresignedAzureURL(AzureUploader{
        ContainerName: containerName,
        VideoId:       videoId,
        Credential:    cloudSession.AzureSession,
    })
default:
    return ""
}

Copy link
Member Author

Choose a reason for hiding this comment

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

here, we are checking != "", and we can't negate values in switch case, thats why used if

Comment on lines +78 to +81
func GetPreSignedURL(videoId string) string {
cloudSession := configs.GetCloudSession()

containerName := constants.CLOUD_CONTAINER_NAME_TEMP
Copy link
Member

Choose a reason for hiding this comment

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

It would be better if we can Extract the logic for getting the presigned URL into separate functions for each cloud provider, and call the appropriate function based on the cloud provider. For example, you can have a function getPresignedGCPURL() that takes the necessary parameters and returns the presigned URL for GCP, and similarly for AWS and Azure. This way, the code for each cloud provider is separated and more readable.

We can move these cloud specific function to a different file as well for code modularity and readability.

func getPresignedGCPURL(videoId string, containerName string) string {
	// Code to generate the presigned URL for GCP
}

func getPresignedAWSURL(videoId string, containerName string) string {
	// Code to generate the presigned URL for AWS
}

func getPresignedAzureURL(videoId string, containerName string) string {
	// Code to generate the presigned URL for Azure
}

func GetPreSignedURL(videoId string, containerName string) string {
	cloudSession := configs.GetCloudSession()

	switch {
	case configs.EnvVar[configs.GCP_PROJECT_ID] != "":
		return getPresignedGCPURL(videoId, containerName)
	case configs.EnvVar[configs.AWS_ACCESS_KEY_ID] != "":
		return getPresignedAWSURL(videoId, containerName)
	case configs.EnvVar[configs.AZURE_ACCESS_KEY] != "":
		return getPresignedAzureURL(videoId, containerName)
	default:
		return ""
	}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, was trying to do the same and wanted to share the sessions to each of functions, got this was only, will try to improve this in next fix

@abskrj abskrj mentioned this pull request Oct 8, 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.

Add Azure and GCP support for pre-signing url
3 participants