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

Add azure upload and download client logic #27

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

isatotun
Copy link

This is a basic working piece for the upload and download of blob files into and from azure.
The methods here will be extended but for now in terms of testing some development pipelines this should suffice.

@isatotun isatotun requested a review from mcarans April 29, 2024 21:43
Copy link

Test Results

97 tests  ±0   97 ✅ ±0   9s ⏱️ -1s
 1 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 9d4cb9b. ± Comparison against base commit ce79080.

@coveralls
Copy link

Coverage Status

coverage: 97.02%. remained the same
when pulling 9d4cb9b on add-azure-client-logic
into ce79080 on main.

Copy link
Contributor

@mcarans mcarans left a comment

Choose a reason for hiding this comment

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

Looks good to me. I've left a few comments for you to consider.

account: str,
container: str,
key: str,
data: None,
Copy link
Contributor

Choose a reason for hiding this comment

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

It can be helpful to users of the function specify a type hint eg. Optional[Dict] if it can be None or a dictionary

account (str): Storage account to access the blob
container (str): Container to download from
key (str): Key to access the blob
blob (str): Name of the blob to be downloaded. If empty, then it is assumed to download
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the default is None, the type should be Optional[str]

"If-None-Match": "",
"If-Unmodified-Since": "",
"Range": "",
"CanonicalizedHeaders": "x-ms-date:"
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using f-strings for this and other cases of concatenating strings

@isatotun
Copy link
Author

isatotun commented May 2, 2024

I am going to put this PR on hold for now and getting back to it next month.

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.

3 participants