-
Notifications
You must be signed in to change notification settings - Fork 454
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
[pack] extension bundle probing and download #4043
Conversation
@soninaren / @SatishRanjan - Coincidentally we have two features which require probing directories for dependencies. It makes sense to define a common interface for both of them. @fabiocav - thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Submitting the feedback based on the quick run through we had.
Will look at changes once you update.
src/WebJobs.Script/BindingExtensionBundle/ExtensionBundleManager.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/BindingExtensionBundle/ExtensionBundleManager.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/BindingExtensionBundle/ExtensionBundleOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/BindingExtensionBundle/ExtensionBundleOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/BindingExtensionBundle/ExtensionBundleOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/BindingExtensionBundle/ExtensionBundleManager.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added few comments. Let me know when you add the code that consumes these changes.
test/WebJobs.Script.Tests/Configuration/ExtensionBundleOptionsSetupTest.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/BindingExtensionBundle/ExtensionBundleOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/BindingExtensionBundle/ExtensionBundleOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/BindingExtensionBundle/IExtensionBundleManager.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Added few comments. Mostly nits.
@fabiocav - Please take a look at IScriptStartupTypeLocatorFactory changes
src/WebJobs.Script/DependencyInjection/ScriptStartupTypeLocator.cs
Outdated
Show resolved
Hide resolved
@pragnagopa @brettsam @alrod working on a review here, but it would be good to have your feedback |
There was a problem hiding this 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 at this and left some small comments
src/WebJobs.Script/BindingExtensionBundle/ExtensionBundleManager.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/DependencyInjection/IScriptStartupTypeLocatorFactory.cs
Outdated
Show resolved
Hide resolved
@fabiocav have addressed the PR comments. Let me know if there are any more changes. |
Closing this one and opening this one instead #4135. We can reiterate the new PR. This would allow me to create draft PR's that build on top of the branch from the new PR. |
Resolves #3961
Resolves #3711