Conversation
|
Please link a related issue and/or update PR description with details on changes made - thanks! |
alrod
force-pushed
the
alrod/sc-redesign3
branch
from
39af175 to
cce9baf
Compare
| // FunctionsScaleManager as a parameter. | ||
| if (Utility.TryGetHostService(scriptHostManager, out FunctionsScaleManager scaleManager)) | ||
| // ScaleManager as a parameter. | ||
| if (Utility.TryGetHostService(scriptHostManager, out IScaleStatusProvider scaleManager)) |
There was a problem hiding this comment.
naming/comments no longer match type
There was a problem hiding this comment.
nit: one more in Line 330: FunctionsScaleManager does not exist anymore
There was a problem hiding this comment.
Change the name of the parameter to match as well - "scaleStatusProvider"
Can you please follow up on this comment from @liliankasem or provide more information in the PR description? |
| // This case should never happen. Because this action is marked RequiresRunningHost, | ||
| // it's only invoked when the host is running, and if it's running, we'll have access | ||
| // to the FunctionsScaleManager. | ||
| // to the IScriptHostManager. |
There was a problem hiding this comment.
This should be IScaleStatusProvider
| .GetSection(ConfigurationSectionNames.Scale) | ||
| .Bind(o); | ||
| }); | ||
| services.ConfigureOptions<ScaleOptionsSetup>(); |
There was a problem hiding this comment.
I think this looks right now. My main concern is/was whether we'd have any breaking changes with apps that might be using app settings to configure ScaleOptions now, if we're changing the config section names we're binding to. The SDK logic is here and it uses a helper that allows the Functions Host to override the section name which it does here. So I think we're good, I just need you to be sure/confirm :)
There was a problem hiding this comment.
yes, there is no breaking change - the section names remain the same.
| [InlineData(false, true)] | ||
| [InlineData(true, false)] | ||
| [InlineData(false, false)] | ||
| public void ScaleOptionsSetup_Works_As_Expected(bool runtimeScaleMonitoringEnabled, bool targetBaseScalingEnabled) |
There was a problem hiding this comment.
nit: ScaleOptionsSetup_ConfiguresExpectedDefaults
5 participants
Issue describing the changes in this PR
resolves #9182
Pull request checklist
release_notes.mdAdditional information
Additional PR information