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

Ensure kv collection refresh settings are not considered unless the feature is enabled. #633

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jimmyca15
Copy link
Member

@jimmyca15 jimmyca15 commented Mar 5, 2025

This PR does the following:

  • Ensure that kv collection refresh interval is not used unless collection based refresh of key-values is enabled.
  • Fix short circuit code path when no refresh is due.
  • Add tests to ensure that minimum refresh interval is respected for key-values and feature flags.

Impact wise, these changes should have little to no behavioral/performance impact. Mostly code clarity.

In response to #632

…ion based refresh of key-values is enabled.

Add tests to ensure that minimum refresh interval is respected for key-values and feature flags.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR ensures that the key-value collection refresh interval is only applied when collection‑based refresh is enabled and adds tests to verify that both key-value and feature flag refresh intervals are respected.

  • Added tests to confirm that the refresh interval for a single key and feature flags is enforced.
  • Updated the refresh logic in the provider to check that kv collection refresh settings are only used when RegisterAll is enabled.
  • Added a precondition check to throw an exception when an invalid KvCollectionRefreshInterval is provided.

Reviewed Changes

File Description
tests/Tests.AzureAppConfiguration/RefreshTests.cs Added tests for ensuring minimum refresh intervals and verifying that refresh calls behave as expected.
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs Updated refresh logic to consider RegisterAllEnabled and introduced a validation check for KvCollectionRefreshInterval.

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs:121

  • [nitpick] Consider enhancing the exception message to more explicitly state that the KvCollectionRefreshInterval is only applicable when using RegisterAll enabled mode.
if (options.KvCollectionRefreshInterval <= TimeSpan.Zero)
@jimmyca15
Copy link
Member Author

@amerjusupovic it looks like the tests i added already existed. I removed them. But I noticed that tests were using Thread.Sleep, I replaced them with Task.Delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants