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

[pack] extension bundle probing and download #4135

Merged
merged 8 commits into from
Mar 27, 2019
Merged

[pack] extension bundle probing and download #4135

merged 8 commits into from
Mar 27, 2019

Conversation

soninaren
Copy link
Member

@soninaren soninaren commented Feb 25, 2019 •
edited
Loading

Resolves #3711
Resolves #3961
Resolves #3714
Resolves #4061

@soninaren soninaren mentioned this pull request Feb 25, 2019
Closed
@soninaren
Copy link
Member Author

Created a new PR with same content from #4043.

@mattchenderson
Copy link
Contributor

@soninaren What's the status here? Is there anything we're waiting on before reviewers can get going?

@mattchenderson
Copy link
Contributor

@soninaren what are the chances we get this into this next release?

@soninaren
Copy link
Member Author

@soninaren What's the status here? Is there anything we're waiting on before reviewers can get going?

PR is ready for review. I have addressed all the comments requested in the last iteration.

@soninaren what are the chances we get this into this next release?
Need quick feedback loop from @fabiocav , @brettsam , @pragnagopa. The PR has gone through a number of iterations. So chances are pretty good for getting this checked in.

pragnagopa
pragnagopa reviewed Mar 26, 2019
View reviewed changes
Copy link
Member

@pragnagopa pragnagopa left a comment

Choose a reason for hiding this comment

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

Added few comments

src/WebJobs.Script.WebHost/WebScriptHostBuilderExtension.cs Outdated Show resolved Hide resolved
src/WebJobs.Script/BindingExtensions/ExtensionsManager.cs Outdated Show resolved Hide resolved
src/WebJobs.Script/ExtensionBundle/ExtensionBundleManager.cs Outdated Show resolved Hide resolved
src/WebJobs.Script/ExtensionBundle/ExtensionBundleManager.cs Outdated Show resolved Hide resolved
src/WebJobs.Script/DependencyInjection/ScriptStartupTypeLocator.cs Show resolved Hide resolved
src/WebJobs.Script/ExtensionBundle/ExtensionBundleManager.cs Show resolved Hide resolved
pragnagopa
pragnagopa reviewed Mar 26, 2019
View reviewed changes
src/WebJobs.Script.WebHost/Controllers/ExtensionsController.cs
@@ -58,6 +62,11 @@ public async Task<IActionResult> Get()
[Route("admin/host/extensions/{id}")]
public async Task<IActionResult> Delete(string id)
{
if (_extensionBundleManager.IsExtensionBundleConfigured())
{
Copy link
Member

Choose a reason for hiding this comment

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

As discussed in person, please add tests

Copy link
Member Author

Choose a reason for hiding this comment

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

There aren't any tests cases for any of the controller yet. We might need to setup something in e2e tests. Have added test cases for changes to extension manager in the extension manager tests.

Copy link
Member

Choose a reason for hiding this comment

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

The scenarios based tests would be a good place for this. Some of the examples we have are the keys controller tests.

pragnagopa
pragnagopa reviewed Mar 26, 2019
View reviewed changes
Copy link
Member

@pragnagopa pragnagopa left a comment

Choose a reason for hiding this comment

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

Added few minor comments.

fabiocav
fabiocav reviewed Mar 26, 2019
View reviewed changes
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.

Left a few comments

src/WebJobs.Script.WebHost/Controllers/ExtensionsController.cs
@@ -58,6 +62,11 @@ public async Task<IActionResult> Get()
[Route("admin/host/extensions/{id}")]
public async Task<IActionResult> Delete(string id)
{
if (_extensionBundleManager.IsExtensionBundleConfigured())
{
Copy link
Member

Choose a reason for hiding this comment

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

The scenarios based tests would be a good place for this. Some of the examples we have are the keys controller tests.

src/WebJobs.Script.WebHost/Controllers/ExtensionsController.cs Show resolved Hide resolved
src/WebJobs.Script/ScriptHostBuilderExtensions.cs Outdated Show resolved Hide resolved
src/WebJobs.Script/BindingExtensions/ExtensionsManager.cs Outdated Show resolved Hide resolved
src/WebJobs.Script/Config/ExtensionBundleOptionsSetup.cs Outdated Show resolved Hide resolved
src/WebJobs.Script/ExtensionBundle/ExtensionBundleManager.cs Outdated Show resolved Hide resolved
src/WebJobs.Script/ExtensionBundle/ExtensionBundleManager.cs Show resolved Hide resolved
src/WebJobs.Script/ExtensionBundle/IExtensionBundleManager.cs Outdated Show resolved Hide resolved
test/WebJobs.Script.Tests/Configuration/ExtensionBundleOptionsSetupTest.cs Outdated Show resolved Hide resolved
test/WebJobs.Script.Tests/ExtensionBundleManagerTests.cs Outdated Show resolved Hide resolved
pragnagopa
pragnagopa reviewed Mar 27, 2019
View reviewed changes
src/WebJobs.Script/ScriptHostBuilderExtensions.cs Outdated
{
var options = new ExtensionBundleOptions();
var optionsSetup = new ExtensionBundleOptionsSetup(context.Configuration, SystemEnvironment.Instance, hostingEnvironment);
var optionsSetup = new ExtensionBundleOptionsSetup(context.Configuration, SystemEnvironment.Instance, context.HostingEnvironment);
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

It's a bit confusing that this is an IOptionsSetup<ExtensionBundleOptions> but not really used as one (not registered, just explicitly used here). I can see that confusing others if they're working on this code. We might want to just drop that implementation and rename this to reflect that this is a configuration helper.

@fabiocav
Copy link
Member

@soninaren just ping when this is ready for review

fabiocav
fabiocav approved these changes Mar 27, 2019
View reviewed changes
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.

Just a couple of very small comments. Once those addressed, I'm good with this.

src/WebJobs.Script/ScriptHostBuilderExtensions.cs Outdated
{
var options = new ExtensionBundleOptions();
var optionsSetup = new ExtensionBundleOptionsSetup(context.Configuration, SystemEnvironment.Instance, hostingEnvironment);
var optionsSetup = new ExtensionBundleOptionsSetup(context.Configuration, SystemEnvironment.Instance, context.HostingEnvironment);
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit confusing that this is an IOptionsSetup<ExtensionBundleOptions> but not really used as one (not registered, just explicitly used here). I can see that confusing others if they're working on this code. We might want to just drop that implementation and rename this to reflect that this is a configuration helper.

src/WebJobs.Script/Environment/EnvironmentSettingNames.cs Outdated
@@ -53,6 +53,6 @@ public static class EnvironmentSettingNames
public const string LinuxAzureAppServiceStorage = "WEBSITES_ENABLE_APP_SERVICE_STORAGE";
public const string CoreToolsEnvironment = "FUNCTIONS_CORETOOLS_ENVIRONMENT";

public const string AlternateCdnUri = "FUNCTIONS_ALTERNATE_CDN_URI";
public const string FunctionsFallbackCdnUri = "FUNCTIONS_FALLBACK_CDN_URI";
Copy link
Member

Choose a reason for hiding this comment

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

Rename this as discussed.

pragnagopa
pragnagopa approved these changes Mar 27, 2019
View reviewed changes
Copy link
Member

@pragnagopa pragnagopa left a comment

Choose a reason for hiding this comment

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

LGTM!

@soninaren soninaren merged commit bb26fc3 into dev Mar 27, 2019
brettsam pushed a commit that referenced this pull request Apr 5, 2019
@soninaren @brettsam
[pack] extension bundle probing, download and extesion management (#4135
fe9d31b 
)

extensionBundle  probing, download and extesion management
@soninaren soninaren deleted the bundleManagement branch May 3, 2019 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pragnagopa pragnagopa pragnagopa approved these changes

@fabiocav fabiocav fabiocav approved these changes

@brettsam brettsam Awaiting requested review from brettsam

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

Successfully merging this pull request may close these issues.
4 participants
@soninaren @mattchenderson @fabiocav @pragnagopa