Add host.json for sync trigger payload #7386

TsuyoshiUshio · 2021-05-19T23:18:23Z

Under the Kubernetes Environment, We need to host.json payload for SyncTrigger scenario for transforming Durable Functions and LogicApp scenario.

I currently testing this branch not finished, so that I make it Draft, However, this change should requires discussion with us. I just simply add the host.json payload on the current sync trigger payload.

CC @pragnagopa , @ahmelsayed

Issue describing the changes in this PR

resolves #7288 #7382

Pull request checklist

My changes do not require documentation changes
My changes should not be added to the release notes for the next release
- Otherwise: I've added my notes to release_notes.md
My changes do not need to be backported to a previous version
- Otherwise: Backport tracked by issue/PR #issue_or_pr
I have added all required tests (Unit tests, E2E tests)

cgillum

This looks good to me, but let's also get a sign-off from @mathewc

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs

Co-authored-by: Chris Gillum <cgillum@microsoft.com>

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs

mathewc · 2021-05-20T17:35:12Z

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs

+            // Add host.json to the payload
+            if (_environment.IsKubernetesManagedHosting())
+            {
+                result.Add("host.json", GetHostJson());


See the comment I left in the previous iteration. Looks like a revision you posted blew the comment away.

Let's not make this Kubernetes specific, since part of our larger goal is to address #7288. Instead, I think we should add just the host.json "extensions" section here, and below if we revert to minimal format based on size, we redefine the minimal format as { triggers: [], extensions: {} } ensuring this data is present in that case as well.

Thank you @mathewc
Done.
Could you have a look?

Looks good. I just had one more comment that I added on removing the unnecessary log statement. They we can get this checked in after you add a test for the scenario - I believe you're in the process of adding one?

Thank you @mathewc
I tested E2E, so that code is good for now, I'm planning add unit testing for it.
Do we need E2E testing as well? If so I'd happy to do that.

For the scope of the changes here unit test is enough! Thanks!

I added an Unit Test. Looks good?

I don’t see updates to unit tests. Missed a commit?

Hi @pragnagopa
Sorry, I missed. Now added.

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs

TsuyoshiUshio · 2021-05-20T18:53:29Z

/azp run

azure-pipelines · 2021-05-20T18:53:52Z

Azure Pipelines successfully started running 1 pipeline(s).

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs

…t is exists

TsuyoshiUshio · 2021-05-20T22:34:23Z

Hi @mathewc
I made change as you requested. Thank you for the quick response and great review!

TsuyoshiUshio · 2021-05-20T22:54:18Z

I'm testing this PR E2E once it finished, I'll share it.

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs

test/WebJobs.Script.Tests/Managment/FunctionsSyncManagerTests.cs

mathewc · 2021-05-21T15:21:25Z

test/WebJobs.Script.Tests/Managment/FunctionsSyncManagerTests.cs

+
+namespace Microsoft.Azure.WebJobs.Script.Tests.Managment
+{
+    public class FunctionsSyncManagerTests


Why did you add a new file for this test - we already have an existing test suite here

azure-functions-host/test/WebJobs.Script.Tests.Integration/Management/FunctionsSyncManagerTests.cs

Line 31 in 6cef580

public class FunctionsSyncManagerTests : IDisposable

.

In the upcoming PR, let's fix this. Also, the test doesn't really test the feature/changes you added - ensuring that the actual sync triggers payload contains the expected content.

Thanks @mathewc - captured notes here: #7394

Add host.json for sync trigger payload

739750e

cgillum previously approved these changes May 19, 2021

View reviewed changes

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs Outdated Show resolved Hide resolved

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs Outdated Show resolved Hide resolved

cgillum requested a review from mathewc May 19, 2021 23:35

Change the message format

4735891

Co-authored-by: Chris Gillum <cgillum@microsoft.com>

TsuyoshiUshio dismissed cgillum’s stale review via 4735891 May 19, 2021 23:36

pragnagopa requested a review from divyagandhisethi May 19, 2021 23:54

pragnagopa added this to the Functions Sprint 100 milestone May 19, 2021

TsuyoshiUshio added 5 commits May 19, 2021 17:14

change message in case of parse error

91f125c

Update the index for the logging test

c193c87

Investigate the log structure

a54fc3a

Add logs for investigating test failure

86971bf

Narrow the scope the end to end to kubernetes environment

9549dcc

mathewc reviewed May 20, 2021

View reviewed changes

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs Outdated Show resolved Hide resolved

mathewc reviewed May 20, 2021

View reviewed changes

remove unnecessary comment

81aba1b

divyagandhisethi reviewed May 20, 2021

View reviewed changes

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs Show resolved Hide resolved

revert the change

e0c7975

cgillum reviewed May 20, 2021

View reviewed changes

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs Show resolved Hide resolved

mathewc reviewed May 20, 2021

View reviewed changes

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs Outdated Show resolved Hide resolved

TsuyoshiUshio marked this pull request as ready for review May 20, 2021 19:04

mathewc reviewed May 20, 2021

View reviewed changes

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs Outdated Show resolved Hide resolved

mathewc reviewed May 20, 2021

View reviewed changes

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs Outdated Show resolved Hide resolved

TsuyoshiUshio added 3 commits May 20, 2021 13:29

Change the read style for Mock avaiable.

2a113e0

forget to add

203d6de

Update for getting extensions

a40f879

mathewc reviewed May 20, 2021

View reviewed changes

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs Outdated Show resolved Hide resolved

mathewc reviewed May 20, 2021

View reviewed changes

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs Outdated Show resolved Hide resolved

mathewc reviewed May 20, 2021

View reviewed changes

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs Outdated Show resolved Hide resolved

TsuyoshiUshio mentioned this pull request May 20, 2021

Refactor the SyncTrigger extensions payload to make it generic #7394

Closed

Make the method return null and only add the extension section when i…

73791bd

…t is exists

TsuyoshiUshio mentioned this pull request May 20, 2021

Include extensions section from host.joson to create scale triggers Azure-App-Service/KuduLite#209

Merged

mathewc reviewed May 21, 2021

View reviewed changes

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs Outdated Show resolved Hide resolved

pragnagopa reviewed May 21, 2021

View reviewed changes

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs Outdated Show resolved Hide resolved

TsuyoshiUshio added 3 commits May 20, 2021 19:14

Remove unnecessary log

f4d0c91

Add unit testing

1b9257c

Add unit test

a70e57a

pragnagopa reviewed May 21, 2021

View reviewed changes

test/WebJobs.Script.Tests/Managment/FunctionsSyncManagerTests.cs Show resolved Hide resolved

Include Cleanup into finally

9df21d2

pragnagopa approved these changes May 21, 2021

View reviewed changes

pragnagopa merged commit 6cef580 into dev May 21, 2021

pragnagopa deleted the tsushi/addhostjson branch May 21, 2021 04:38

pragnagopa pushed a commit that referenced this pull request May 21, 2021

Add extensions config from host.json to sync trigger payload (#7386)

55faf4e

mathewc reviewed May 21, 2021

View reviewed changes

TsuyoshiUshio mentioned this pull request May 25, 2021

SyncTrigger fails to call on Kubernetes Environment #7360

Closed

TsuyoshiUshio mentioned this pull request Nov 16, 2021

Include Extension and Dynamic Concurrency in Sync Trigger Payload #7863

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add host.json for sync trigger payload #7386

Add host.json for sync trigger payload #7386

TsuyoshiUshio commented May 19, 2021

cgillum left a comment

mathewc May 20, 2021

mathewc May 20, 2021

TsuyoshiUshio May 20, 2021

mathewc May 21, 2021

TsuyoshiUshio May 21, 2021

pragnagopa May 21, 2021

TsuyoshiUshio May 21, 2021

pragnagopa May 21, 2021

TsuyoshiUshio May 21, 2021

TsuyoshiUshio commented May 20, 2021

azure-pipelines bot commented May 20, 2021

TsuyoshiUshio commented May 20, 2021

TsuyoshiUshio commented May 20, 2021 •

edited

Loading

mathewc May 21, 2021

pragnagopa May 21, 2021

Add host.json for sync trigger payload #7386

Add host.json for sync trigger payload #7386

Conversation

TsuyoshiUshio commented May 19, 2021

Issue describing the changes in this PR

Pull request checklist

cgillum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TsuyoshiUshio commented May 20, 2021

azure-pipelines bot commented May 20, 2021

TsuyoshiUshio commented May 20, 2021

TsuyoshiUshio commented May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TsuyoshiUshio commented May 20, 2021 •

edited

Loading