V2 SyncTriggers Improvements #4145
Conversation
| @@ -186,11 +189,13 @@ private async Task<IActionResult> AddOrUpdateSecretAsync(string keyName, string | |||
| case OperationResult.Created: | |||
| { | |||
| var keyResponse = ApiModelUtility.CreateApiModel(new { name = keyName, value = operationResult.Secret }, Request); | |||
| await _functionsSyncManager.TrySyncTriggersAsync(); | |||
There was a problem hiding this comment.
Now that keys can also be part of the sync triggers payload, we need to sync triggers whenever they are updated.
mathewc
force-pushed
the
sync-trigger-ex
branch
from
f3298ff
to
27a558c
Compare
| uriString += $"/{routePrefix.TrimEnd('/')}"; | ||
| } | ||
|
|
||
| if (!string.IsNullOrEmpty(customRoute)) |
There was a problem hiding this comment.
There was actually a bug in this code - if a custom route was being used, it was being appended directly to the base route, w/o including the route prefix.
|
|
||
| internal static string GetBaseUrl() | ||
| { | ||
| string hostName = Environment.GetEnvironmentVariable("WEBSITE_HOSTNAME") ?? "localhost"; |
There was a problem hiding this comment.
Previously we were getting the base URL from the active request. However, we're now needing to compute this in the absence of a request (e.g. background sync triggers)
I have a related PR #4338 out for fixing WEBSITE_HOSTNAME
There was a problem hiding this comment.
Should this be using IEnvironment rather than Environment?
There was a problem hiding this comment.
I'll be rebasing on the WEBSITE_HOSTNAME changes that just went in so this will be coming from the HostNameProvider
mathewc
changed the title
mathewc
force-pushed
the
sync-trigger-ex
branch
2 times, most recently
from
3141708
to
06c0a18
Compare
mathewc
force-pushed
the
sync-trigger-ex
branch
from
06c0a18
to
2c86877
Compare
mathewc
changed the title
mathewc
force-pushed
the
sync-trigger-ex
branch
3 times, most recently
from
707d00b
to
d1ae737
Compare
| // Add functions secrets to the payload | ||
| // Only secret types we own/control can we cache directly | ||
| // Encryption is handled by Antares before storage | ||
| var secretsStorageType = Environment.GetEnvironmentVariable(EnvironmentSettingNames.AzureWebJobsSecretStorageType); |
There was a problem hiding this comment.
should this go against _environment instead of Environment?
| @@ -59,6 +65,14 @@ public FunctionsSyncManager(IConfiguration configuration, IHostIdProvider hostId | |||
| _environment = environment; | |||
| } | |||
There was a problem hiding this comment.
nit -- can we change this to take an ILogger<FunctionsSyncManager> in the ctor?
There was a problem hiding this comment.
If we change the logger category like this, it'll make detectors easier to run as they can filter on it. @FinVamp1 FYI.
|
|
||
| internal static string GetBaseUrl() | ||
| { | ||
| string hostName = Environment.GetEnvironmentVariable("WEBSITE_HOSTNAME") ?? "localhost"; |
There was a problem hiding this comment.
Should this be using IEnvironment rather than Environment?
mathewc
force-pushed
the
sync-trigger-ex
branch
from
d1ae737
to
8c7154c
Compare
Addresses #3949. See corresponding v1 PR: #4325.
Because the extended sync triggers payload format is behind a feature flag (WEBSITE_FUNCTIONS_ARMCACHE_ENABLED) these changes can be released before ANT 82. We'll just enable the extended format post ANT 82 by flipping the default in a new runtime release.