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

auth: Make getSession synchronous #1619

Merged
merged 4 commits into from
Nov 3, 2023
Merged

auth: Make getSession synchronous #1619

merged 4 commits into from
Nov 3, 2023

Conversation

nturinski
Copy link
Member

Actually fixes: microsoft/vscode-azurefunctions#3878

This PR broke everything. 😡

Apparently AzureAuthentication doesn't like getSession being async. So we need to be able to get the session and pass that along before we create the authentication object.

The async call must have somehow caused some parts of the zipfile not to get deployed since the stream was constantly making requests. Not sure why it only reproduces on Windows either. I'm assuming the auth provider was fast enough on a Mac to never actually be an issue.

Anyway, I'm making scopes an optional parameter on createSubscriptionClient. I think that should resolve the problem of not being able to pass scopes.

Also could someone double check to make sure that I reverted everything? I think that this is all, but my sanity bar is nearly depleted atm.

@motm32
Copy link
Contributor

motm32 commented Nov 2, 2023

Can confirm (after multiple tries) this does in fact fix.

bwateratmsft
bwateratmsft previously approved these changes Nov 3, 2023
auth/package.json Outdated Show resolved Hide resolved
@bwateratmsft
Copy link
Contributor

Also could someone double check to make sure that I reverted everything? I think that this is all, but my sanity bar is nearly depleted atm.

Don't Starve intensifies

alexweininger
alexweininger previously approved these changes Nov 3, 2023
Copy link
Member

@alexweininger alexweininger left a comment

Choose a reason for hiding this comment

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

awkward

But also yeah we shouldn't need cross-fetch or node-fetch now that we can use the build-in fetch. I did that in #1614, so just copy over those changes to here.

@nturinski nturinski dismissed stale reviews from alexweininger and bwateratmsft via 48cff30 November 3, 2023 16:19
@nturinski nturinski merged commit 778b214 into main Nov 3, 2023
4 checks passed
@nturinski nturinski deleted the auth/nat/sessionFix branch November 3, 2023 18:15
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.

Fail to deploy a project to a function app with an error
4 participants