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

[Datastore] Extend Azure blob to support other auth methods #1140

Merged
merged 24 commits into from Jul 26, 2021

Conversation

hayesgb
Copy link
Contributor

@hayesgb hayesgb commented Jul 23, 2021

Currently, the only supported authentication methods against AzureBlobStore is AZURE_STORAGE_CONNECTION_STRING. , and possibly AZURE_STORAGE_KEY. This enables other authentication methods including use of ServicePrincipals and SAS tokens.

…dentials other than connection_string or account_key
…if not using Pandas, since Dask requires storage_options. Also changed line mlrun#226 from inplace=True to inplace=False.  Dask does not support dropping columns inplace.  Alternatively, we could test if the df is hasattr 'dask' and only apply if inplace, but this is a more general behavior.
@hayesgb
Copy link
Contributor Author

hayesgb commented Jul 23, 2021

Proposed fix for #1141

@Hedingber Hedingber requested a review from theSaarco July 25, 2021 00:35
@Hedingber
Copy link
Contributor

Hi @hayesgb
Thanks a lot for the contribution!
I asked @theSaarco to review it, in the meanwhile, I see that the lint CI failed,
Did you follow the instructions on https://github.com/mlrun/mlrun/blob/development/CONTRIBUTING.md and specifically running make fmt and make lint ?
Let me know if you're having any trouble with this

@theSaarco
Copy link
Member

@hayesgb, thank you for contributing.
In terms of testing, we have a manual integration test located in tests/integration/azure_blob/test_azure_blob.py - these tests are manual since they require you to provide access parameters for the Azure storage account you wish to test against. These details are given in a yml file in the same directory.
Given that you've added new access modes which use different parameters, please add the option to specify these parameters in this test, so it can be used to test your new additions. Please let me know if you need assistance for this.

mlrun/datastore/azure_blob.py Outdated Show resolved Hide resolved
mlrun/datastore/azure_blob.py Outdated Show resolved Hide resolved
mlrun/datastore/azure_blob.py Show resolved Hide resolved
mlrun/datastore/azure_blob.py Show resolved Hide resolved
tests/integration/azure_blob/test_azure_blob.py Outdated Show resolved Hide resolved
tests/integration/azure_blob/test_azure_blob.py Outdated Show resolved Hide resolved
tests/integration/azure_blob/test_azure_blob.py Outdated Show resolved Hide resolved
tests/integration/azure_blob/test_azure_blob.py Outdated Show resolved Hide resolved
mlrun/datastore/azure_blob.py Outdated Show resolved Hide resolved
mlrun/datastore/azure_blob.py Outdated Show resolved Hide resolved
hayesgb and others added 5 commits July 26, 2021 05:52
Co-authored-by: Saar Cohen <66667568+theSaarco@users.noreply.github.com>
Co-authored-by: Saar Cohen <66667568+theSaarco@users.noreply.github.com>
Co-authored-by: Saar Cohen <66667568+theSaarco@users.noreply.github.com>
Co-authored-by: Saar Cohen <66667568+theSaarco@users.noreply.github.com>
@hayesgb
Copy link
Contributor Author

hayesgb commented Jul 26, 2021

As a side-note, when I try to run make test-dockerized on my local machine, I'm getting the following error:

#5 1.083 E: The repository 'http://security.debian.org/debian-security buster/updates InRelease' is not signed. #5 1.083 W: GPG error: http://deb.debian.org/debian buster-updates InRelease: At least one invalid signature was encountered. #5 1.083 E: The repository 'http://deb.debian.org/debian buster-updates InRelease' is not signed.

Do you recommend I assume this is trusted?

@Hedingber
Copy link
Contributor

@hayesgb I've never encountered this error, googling it brought up this one https://stackoverflow.com/questions/62473932/atleast-one-invalid-signature-was-encountered
I would start with the suggestions there (Cleaning caches and images) before manually "making it trusted"
BTW, note that the reason CI doesn't run automatically on all of your commits (it's running only when I approve) is because you're a first time contributor, once we'll merge one of the PRs, it will be more comfortable.
Another thing on that regard, CI passed successfully 🥳

@hayesgb
Copy link
Contributor Author

hayesgb commented Jul 26, 2021

Thanks @Hedingber -- Do I need to make any additional changes at this point? Can we consider the requested azure_connection_configured() update completed?

@theSaarco
Copy link
Member

@hayesgb - I'm fine with the changes done so far. I have some ideas for improving the test further, but no reason to block this PR for them at this point. 🎉

@Hedingber Hedingber changed the title Extend DataStore operations on Azure [Datastore] Extend Azure blob to support other auth methods Jul 26, 2021
@Hedingber Hedingber merged commit 602c38d into mlrun:development Jul 26, 2021
@hayesgb hayesgb deleted the enable_abfs branch July 26, 2021 14:33
@Hedingber
Copy link
Contributor

@hayesgb
PR was merged, you're officially an MLRun contributor now 🎉

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.

None yet

3 participants