Skip to content

Conversation

safihamid
Copy link
Contributor

work in progress...

{
get
{
return string.Equals(Environment.GetEnvironmentVariable(EnvironmentSettingNames.AzureWebJobsDisableHomepage),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer this to be done via IConfiguration approach so we're not locked to environment variables as the only method of setting this.


if (rc.RouteData != null && rc.RouteData.DataTokens != null)
{
functionName = rc.RouteData.DataTokens["AZUREWEBJOBS_FUNCTIONNAME"].ToString();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should avoid string literals and use a constant

@davidparker
Copy link

hi

Any idea when this might make it onto azure portal? really need proxies to be working, but I can get by if could have some idea if this will be resolved within the next month or so at least?

many thanks

@safihamid
Copy link
Contributor Author

Yes it should be. Waiting for some code reviews.

@safihamid safihamid force-pushed the proxycore branch 2 times, most recently from 5844da0 to 5a88149 Compare March 14, 2018 16:55
Copy link
Member

@fabiocav fabiocav left a comment

Choose a reason for hiding this comment

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

Took a quick look (with you :) ) and left some notes

result = await GetResultAsync(context, functionExecution);

// TODO: need some cleanup
if (nestedProxies != null && int.Parse(nestedProxies.ToString()) > 0)
Copy link
Member

Choose a reason for hiding this comment

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

What is this here?

IFunctionExecutionFeature functionExecution = context.Features.Get<IFunctionExecutionFeature>();

object nestedProxies;
context.Items.TryGetValue("X_MS_NestedProxyCount", out nestedProxies);
Copy link
Member

Choose a reason for hiding this comment

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

Remove this?

return;
}
}
else if (functionExecution == null
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move this to its own middleware? (e.g. HomepageMiddleware), in the future, that also gives us the ability to inspect the flag early and not register the logic.

foreach (var router in route.Routers)
{
var webJobsRouter = router as WebJobsRouter;
if (webJobsRouter != null)
Copy link
Member

Choose a reason for hiding this comment

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

nit: (router is WebJobsRouter webJobsRouter)


FunctionInvocationMiddleware functionInvocationMiddleware = new FunctionInvocationMiddleware(null);

// TODO: need some cleanup
Copy link
Member

Choose a reason for hiding this comment

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

Still need this?

httpContext.Items.Remove("X_MS_ReverseRoutes");
httpContext.Items.Remove(ScriptConstants.AzureFunctionsHostManagerKey);

await rc.Handler.Invoke(httpContext);
Copy link
Member

Choose a reason for hiding this comment

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

Handler could potentially be null here, right?

@safihamid safihamid force-pushed the proxycore branch 4 times, most recently from 28f29ad to 8b63b9d Compare March 30, 2018 15:35
Copy link
Member

@fabiocav fabiocav left a comment

Choose a reason for hiding this comment

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

Small comment. Can you just update that and :shipit: ?

Copy link
Member

Choose a reason for hiding this comment

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

We should be able to move this block to the above if block. result should also never be null.

@safihamid safihamid merged commit 13f0d11 into dev Apr 1, 2018
@safihamid safihamid deleted the proxycore branch April 1, 2018 20:18
@JonathanGiles
Copy link
Member

Does this mean proxies now work in v2 functions, or are there more things to do (e.g. a new release)? Where will the availability be announced (so I don't need to keep asking)? Thanks!

@safihamid
Copy link
Contributor Author

@JonathanGiles Yes they will work with our next release of Functions V2. We usually have a release every couple of weeks.

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.

5 participants