Skip to content
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

Fixed error on documentation tabs #122362

Closed
wants to merge 1 commit into from

Conversation

fjbelizon
Copy link

The tabs "In-Process" and "Isolated" were changed. The error is fixed with this changes.

The tabs "In-Process" and "Isolated" were changed. The error is fixed with this changes.
Copy link
Contributor

@fjbelizon : Thanks for your contribution! The author(s) have been notified to review your proposed change.

Copy link
Contributor

Learn Build status updates of commit b527e5c:

✅ Validation status: passed

File Status Preview URL Details
articles/azure-app-configuration/enable-dynamic-configuration-azure-functions-csharp.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

@Court72
Copy link
Contributor

Court72 commented May 9, 2024

@zhenlan

Can you review the proposed changes?

When the changes are ready for publication, add a #sign-off comment to signal that the PR is ready for the review team to merge.

#label:"aq-pr-triaged"
@MicrosoftDocs/public-repo-pr-review-team

@prmerger-automator prmerger-automator bot added the aq-pr-triaged tracking label for the PR review team label May 9, 2024
@zhenlan
Copy link
Contributor

zhenlan commented May 9, 2024

@fjbelizon can you please elaborate the reason for the change? What's the error?

@fjbelizon
Copy link
Author

@zhenlan yes, of course. The tabs are changed. The information specifies, for example, the use of ‘ConfigureFunctionsWorkerDefaults()’ in Isolated Functions, however in this type of functions it is recommended to use ‘ConfigureFunctionsWebApplication()’ (at least if you use ASP.NET Core integration). Another problem is that the function ‘ConfigureFunctionsWorkerDefaults(app => app.UseAzureAppConfiguration())’ generates debugging problems, in functions that make use of context, such as Eternal Orchestration, etc. This is why the information in the ‘Isolated’ and ‘In-Proc’ tabs is wrong, the ‘In-Proc’ is the ‘Isolated’ and the ‘Isolated’ is the ‘In-Proc’.

@zhenlan
Copy link
Contributor

zhenlan commented May 14, 2024

@fjbelizon thanks. I'm not sure the content should be swapped. But before I dive into it, is this related to the issue you reported here Azure/AppConfiguration#915?

@fjbelizon
Copy link
Author

Yes @zhenlan, the problems are related

@zhenlan
Copy link
Contributor

zhenlan commented May 16, 2024

@fjbelizon thanks for the confirmation. That issue is related to the durable functions. As discussed in Azure/AppConfiguration-DotnetProvider#484, it's more of an issue with middleware in durable.

To work around the issue, you can skip the call to UseAzureAppConfiguration, but get the refresher instance from the DI and call TryRefresh at the beginning of your function calls. This may be the reason you feel this document should swap the content for in-proc and isolated modes. There are far more differences for in-proc vs. isolated modes. If so, I believe this document is correct as is.

@fjbelizon
Copy link
Author

I do not think so. That is why I have opened the documentation issue and the middleware issue. You can take whatever solution you want, I have reported the problem that exists. Thank you very much.

@American-Dipper
Copy link
Contributor

@zhenlan - Should this PR remain open or can it be closed?

Thanks, PR review team

@zhenlan
Copy link
Contributor

zhenlan commented Jun 13, 2024

@American-Dipper this PR can be closed. I may see if there is any improvement/clarification I can make to this document later.

@Court72 Court72 closed this Jun 14, 2024
@fjbelizon
Copy link
Author

I hope it will be done, because the documentation is not correct.

@fjbelizon fjbelizon deleted the patch-1 branch June 19, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants