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.

Sign up for GitHub

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

Jump to bottom

Fixups to AzureFileShareService auth token #819

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

Fixups to AzureFileShareService auth token #819

wants to merge 32 commits into from

Conversation

bpkroth
Copy link
Contributor

@bpkroth bpkroth commented Jul 24, 2024 •
edited
Loading

Makes config errors load time apparent instead of runtime apparent.

Closes #818

motus and others added 24 commits July 8, 2024 16:05
@motus
use token-based credential instead of a storage key in Azure file sha…
2a6cd0d 
…re service
@motus
initialize auth service before azure fileshare in unit tests
951fdba
@motus
late initialization for azure fileshare _share_client; proper monkey …
9aeaa92 
…patching of the class for unit tests
@motus
remove storageAccountKey parameter from azure_fileshare fixture
54c308b
@motus
Merge branch 'main' into sergiym/svc/fs_token_auth
9b65bd7
@motus
Merge branch 'main' into sergiym/svc/fs_token_auth
101b593
@motus
Merge branch 'main' into sergiym/svc/fs_token_auth
71fae3c
@motus
Merge branch 'main' of https://github.com/microsoft/MLOS into sergiym…
4bc4c25 
…/svc/fs_token_auth
@motus
Merge branch 'sergiym/svc/fs_token_auth' of https://github.com/motus/…
289af50 
…MLOS into sergiym/svc/fs_token_auth
@motus
black formatting updates
ead96a1
@motus
Merge branch 'main' into sergiym/svc/fs_token_auth
a46bcd4
@motus
Merge branch 'main' into sergiym/svc/fs_token_auth
68c6d0d
@motus
Merge branch 'main' of https://github.com/microsoft/MLOS into sergiym…
99172ed 
…/svc/fs_token_auth
@motus
docformatter fixes
8078d70
@motus
Merge branch 'sergiym/svc/fs_token_auth' of https://github.com/motus/…
527ae66 
…MLOS into sergiym/svc/fs_token_auth
@motus
Merge branch 'main' into sergiym/svc/fs_token_auth
56704f6
@bpkroth
Merge branch 'main' into sergiym/svc/fs_token_auth
5af8b6f
@motus
Merge branch 'main' into sergiym/svc/fs_token_auth
378f073
@motus
formatting fixes
f2b47c6
@motus
remove git merge artifact
e9e48f2
@motus
more git merge fixes
2925ebb
@bpkroth
move sanity check into __init__
00ff3bf
@bpkroth
Merge remote-tracking branch 'origin/sergiym/svc/fs_token_auth' into …
fd45a8c 
…fixes-to-pr-779

Plus other fixes
@bpkroth
use the mock auth service
7895fe5
@bpkroth bpkroth requested a review from a team as a code owner July 24, 2024 20:27
@bpkroth bpkroth requested a review from motus July 24, 2024 20:27
@motus
Copy link
Member

motus commented Aug 3, 2024

I assume this update went to #820

@motus motus closed this Aug 3, 2024
@bpkroth
Copy link
Contributor Author

bpkroth commented Aug 8, 2024

I assume this update went to #820

Parts of it did, but I don't think all of it. Especially the refresh logic.

@eujing I see that you added some of the test fixes in. Have you tested #820 to confirm that it already handles credential refresh as necessary?

@bpkroth bpkroth reopened this Aug 8, 2024
@bpkroth bpkroth mentioned this pull request Aug 8, 2024
Merged
@bpkroth bpkroth enabled auto-merge (squash) August 8, 2024 22:04
@eujing
Copy link
Contributor

eujing commented Sep 4, 2024

I assume this update went to #820

Parts of it did, but I don't think all of it. Especially the refresh logic.

@eujing I see that you added some of the test fixes in. Have you tested #820 to confirm that it already handles credential refresh as necessary?

Yes I have been using the #820 changes, along with #837 for long running experiments. So at least for DefaultAzureCredential with the MI setup, the token refresh has been handled as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@motus motus Awaiting requested review from motus

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix issues in AzureFileShareService introduced by #779
3 participants
@bpkroth @motus @eujing