-
Notifications
You must be signed in to change notification settings - Fork 464
[in-proc] Fix sync trigger error when AzureWebjobsStorage is not set in Managed #11214
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: in-proc
Are you sure you want to change the base?
Conversation
…App Environment (#10767) * Fix sync trigger error when AzureWebjobsStorage is not set in case of ManagedAppEnvironment for Hybrid Logic Apps Issue : #10686 * Add test * Update release notes * Update release_notes.md * Resolved comments * Fixed comment * fix comment - skip hash check when client is not set * update test for managedenv * add test for kubernetes managed env * resolve comments - to use cache disabled in test --------- Co-authored-by: Ishank Gupta <[email protected]>
* fixed and enabled sync trigger tests for k8s environment * resolve comments * correct test --------- Co-authored-by: Ishank Gupta <[email protected]> (cherry picked from commit 4c4d44b)
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 fixes a sync trigger error that occurs in managed environments when the AzureWebJobsStorage
connection string is not configured. The changes ensure that sync trigger operations can succeed in Kubernetes-based managed hosting scenarios even without Azure blob storage connectivity.
- Modified logic to properly handle managed environments without
AzureWebJobsStorage
- Updated conditional checks to use
IsAnyKubernetesEnvironment()
instead ofIsKubernetesManagedHosting()
- Simplified hash checking logic to depend on blob client availability rather than environment type
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs | Updated sync trigger logic to handle managed environments without Azure storage |
test/WebJobs.Script.Tests.Integration/Management/FunctionsSyncManagerTests.cs | Added comprehensive test coverage for both managed and Kubernetes environments without storage |
release_notes.md | Added entry documenting the fix for managed environment sync triggers |
[Fact] | ||
public async Task TrySyncTriggers_ManagedAppEnv_WithNo_AzureWebJobsStorage_ReturnsTrue() | ||
{ | ||
_vars.Add("AzureWebJobsStorage", null); |
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.
Setting a dictionary value to null may not have the intended effect of removing the environment variable. Consider using an empty string or a specific test utility method to simulate the absence of this environment variable.
_vars.Add("AzureWebJobsStorage", null); | |
_vars.Add("AzureWebJobsStorage", string.Empty); |
Copilot uses AI. Check for mistakes.
[Fact] | ||
public async Task TrySyncTriggers_KubernetesManagedEnv_WithNo_AzureWebJobsStorage_ReturnsTrue() | ||
{ | ||
_vars.Add("AzureWebJobsStorage", null); |
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.
Setting a dictionary value to null may not have the intended effect of removing the environment variable. Consider using an empty string or a specific test utility method to simulate the absence of this environment variable.
_vars.Add("AzureWebJobsStorage", null); | |
_vars.Add("AzureWebJobsStorage", ""); |
Copilot uses AI. Check for mistakes.
Issue describing the changes in this PR
resolves #issue_for_this_pr
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