Skip to content

Conversation

@hassiebp
Copy link
Contributor

@hassiebp hassiebp commented Nov 25, 2024

Important

Adds x-ms-blob-type: BlockBlob header in media_manager.py for Azure Blob Storage support.

  • Azure Blob Storage Support:
    • Adds x-ms-blob-type: BlockBlob header in _process_upload_media_job() in media_manager.py to support Azure Blob Storage uploads.

This description was created by Ellipsis for ecf6edb. It will automatically update as commits are pushed.

@hassiebp hassiebp merged commit 08298f6 into main Nov 25, 2024
4 of 9 checks passed
@hassiebp hassiebp deleted the add-azure-support branch November 25, 2024 10:44
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Disclaimer: Experimental PR review

PR Summary

Added Azure Blob Storage support in the media upload process, though there are concerns with header management across different storage providers.

  • Added Azure-specific header x-ms-blob-type: BlockBlob in /langfuse/_task_manager/media_manager.py for Azure Blob Storage compatibility
  • Potential issue with AWS header x-amz-checksum-sha256 being sent alongside Azure headers
  • Missing provider-specific header logic to handle different storage backends appropriately
  • Risk of upload failures due to mixed cloud storage headers being sent simultaneously

1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines 201 to +203
"Content-Type": data["content_type"],
"x-amz-checksum-sha256": data["content_sha256_hash"],
"x-ms-blob-type": "BlockBlob",
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: AWS and Azure headers are mixed together. Should conditionally set headers based on the storage provider to avoid sending AWS headers to Azure and vice versa.

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.

2 participants