Add SqlOptions for reading extra configuration#960
Conversation
MaddyDev
requested review from
Charles-Gagnon,
VasuBhog,
chlafreniere and
lucyzhang929
as code owners
|
@MaddyDev Would suggest you create follow-up issues for the two things you called out in the description (and then link them there too) |
chlafreniere
changed the title
| IReadOnlyList<string> userTableColumns, | ||
| IReadOnlyList<(string name, string type)> primaryKeyColumns, | ||
| ITriggeredFunctionExecutor executor, | ||
| SqlOptions sqlOptions, |
There was a problem hiding this comment.
Here and below in all types you're taking a SqlOptions directly - make sure you're doing that only for types that aren't being created by DI. You want to use IOptions<SqlOptions> generally for DI created instances which ensures that all config is applied to the options
| this._logger = logger ?? throw new ArgumentNullException(nameof(logger)); | ||
| this._configuration = configuration ?? throw new ArgumentNullException(nameof(configuration)); | ||
| int? configuredMaxChangesPerWorker; | ||
| // TODO: when we move to reading them exclusively from the host options, remove reading from settings. |
There was a problem hiding this comment.
The right way to do this (defaulting your options from env vars) is for you to register an IConfigureOptions as part of your startup. Here's an example from the Functions host https://github.com/Azure/azure-functions-host/blob/6ae170c32c97864d33424e1f8cc1783acdbc907a/src/WebJobs.Script/Config/AppServiceOptionsSetup.cs#L8. It gets registered here https://github.com/Azure/azure-functions-host/blob/6ae170c32c97864d33424e1f8cc1783acdbc907a/src/WebJobs.Script.WebHost/WebScriptHostBuilderExtension.cs#L53. The .NET host builder will compose this along with all the other config providers to apply values in order. You'll want to have tests that verify precedence is followed - e.g. specify values via env vars, simulate host.json as you're doing in some tests, and also via imperative config.
There was a problem hiding this comment.
We'll be removing reading from config at some point (so not going to add more code w.r.t to that). For now, I did add a test for checking precedence (and reading from app settings takes precedence).
* initial set of changes * add comment * remove unwanted ref * add reading from config * use DI * add sqloptions test * rename to SqlExtensionConfigProvider * refactor code * fix tests * fix complie * intialize options * fix input binding test by passing null * add ConfigOverridesHostOptionsTest * remove duplicate test * undo * rename Queues to Sql * address comments
4 participants
Reading our optional configuration values from app settings is not the recommended way and instead using options to read from host.json is. In this PR, I added the SqlOptions to read our optional config values for Sql Trigger from host.json to follow the pattern suggested here
I did not remove the current way of reading from app settings since that would be a breaking change and would warrant a vbump. We can at a later point remove it when we vbump (#961).
Tests - I've added a lot of plumbing code copied over from azure storage sdk for the end to end test. I would revisit and clean that up (removing the unused code) but since we're time bound for GA, I have copied over the entire classes that are needed for set up. (#962)