-
Notifications
You must be signed in to change notification settings - Fork 469
Enabling worker indexing for Logic Apps app kind behind an enviornment setting #11378
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
base: dev
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR enables worker indexing for Logic Apps when running in codeful mode behind an environment variable setting. The change allows Logic Apps to use the dotnet worker runtime with specific environment variables while maintaining backward compatibility with existing Logic Apps that don't use codeful mode.
- Adds
WORKFLOW_CODEFUL_ENABLED
environment variable support to control codeful mode behavior - Modifies worker indexing logic to allow indexing for Logic Apps when codeful mode is enabled
- Disables strict worker runtime validation for Logic Apps in codeful mode
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/WebJobs.Script/Environment/EnvironmentSettingNames.cs | Adds new environment variable constant for Logic App codeful mode |
src/WebJobs.Script/Environment/EnvironmentExtensions.cs | Implements extension method to check if Logic App codeful mode is enabled |
src/WebJobs.Script/Utility.cs | Updates worker indexing logic to allow indexing for Logic Apps in codeful mode |
src/WebJobs.Script/Host/ScriptHost.cs | Disables worker runtime validation for Logic Apps in codeful mode |
/// </summary> | ||
public static bool IsLogicAppCodefulModeEnabled(this IEnvironment environment) | ||
{ | ||
bool.TryParse(environment.GetEnvironmentVariable(LogicAppCodefulModeEnabled), out bool logicAppCodefulModeEnabled); |
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.
Use the full qualified name EnvironmentSettingNames.LogicAppCodefulModeEnabled
instead of the unqualified constant name to improve code clarity and follow C# best practices.
Copilot generated this review using guidance from repository custom instructions.
if (!cancellationToken.IsCancellationRequested) | ||
{ | ||
bool throwOnWorkerRuntimeAndPayloadMetadataMismatch = true; | ||
bool throwOnWorkerRuntimeAndPayloadMetadataMismatch = _environment.IsLogicApp() && _environment.IsLogicAppCodefulModeEnabled() ? false : true; |
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.
The ternary operator ? false : true
is unnecessarily verbose. This can be simplified to !(_environment.IsLogicApp() && _environment.IsLogicAppCodefulModeEnabled())
for better readability.
bool throwOnWorkerRuntimeAndPayloadMetadataMismatch = _environment.IsLogicApp() && _environment.IsLogicAppCodefulModeEnabled() ? false : true; | |
bool throwOnWorkerRuntimeAndPayloadMetadataMismatch = !(_environment.IsLogicApp() && _environment.IsLogicAppCodefulModeEnabled()); |
Copilot uses AI. Check for mistakes.
I have a few questions/comments related to this PR -
Let me know if you need help with any of these items, happy to assist. |
Yeah we plan to use the out of proc model soon so wanted to keep these changes in sync. I have updated the checklist and added github issue and test. |
/// </summary> | ||
public static bool IsLogicAppCodefulModeEnabled(this IEnvironment environment) | ||
{ | ||
bool.TryParse(environment.GetEnvironmentVariable(EnvironmentSettingNames.LogicAppCodefulModeEnabled), out bool logicAppCodefulModeEnabled); |
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.
Which component will be setting this Environment variable?
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.
It will be set by the Logic Apps customers in the app settings manually for now. But later we will make it first class.
// Machine identifier | ||
public const string AntaresComputerName = "COMPUTERNAME"; | ||
|
||
public const string AppKind = "APP_KIND"; |
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.
Remove empty line 157 and add a comment for LA settings -
// Logic apps settings
public const string AppKind = "APP_KIND";
public const string LogicAppCodefulModeEnabled = "WORKFLOW_CODEFUL_ENABLED";
if (!cancellationToken.IsCancellationRequested) | ||
{ | ||
bool throwOnWorkerRuntimeAndPayloadMetadataMismatch = true; | ||
bool throwOnWorkerRuntimeAndPayloadMetadataMismatch = !(_environment.IsLogicApp() && _environment.IsLogicAppCodefulModeEnabled()); |
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.
Not clear on why do we need to make changes here?
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.
Since Logic Apps is using dotnet as the runtime, if you look below this is set to true by default and we would throw an exception here which causes the host startup to fail.
string appName = environment.GetAzureWebsiteUniqueSlotName(); | ||
|
||
bool workerIndexingFeatureFlagSet = FeatureFlags.IsEnabled(ScriptConstants.FeatureFlagEnableWorkerIndexing, environment); | ||
bool workerIndexingDisabledViaHostingConfig = functionsHostingConfigOptions.WorkerIndexingDisabledApps.ToLowerInvariant().Split("|").Contains(appName); |
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.
For scenario, if (environment.IsLogicApp() && environment.IsLogicAppCodefulModeEnabled())
, below line will be executed -
bool workerIndexingDisabledViaHostingConfig = functionsHostingConfigOptions.WorkerIndexingDisabledApps.ToLowerInvariant().Split("|").Contains(appName);
This line is for Functions apps for which indexing is disabled via hosting config. Let's say indexing is disabled for function app "app1" and there is a logic app with the same name, then indexing will be disabled for that LA too. But our requirement is to enable indexing for above scenario.
Fix -
bool workerIndexingDisabledViaHostingConfig = !environment.IsLogicApp() && functionsHostingConfigOptions.WorkerIndexingDisabledApps.ToLowerInvariant().Split("|").Contains(appName);
Add/update existing tests to cover this scenario.
This is to enable worker indexing for Logic Apps workflow app kind behind an environment variable. The settings used by Logic Apps are "FUNCTIONS_INPROC_NET8_ENABLED": "1", "FUNCTIONS_WORKER_RUNTIME": "dotnet" and we are bringing up a "dotnet" worker.
resolves #11386
Pull request checklist
IMPORTANT: Currently, changes must be backported to the
in-proc
branch to be included in Core Tools and non-Flex deployments.in-proc
branch is not requiredrelease_notes.md
Additional information
Additional PR information