Skip to content

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

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

Merged
merged 4 commits into from
Mar 12, 2025

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.
Copy link

@Copilot Copilot AI left a comment

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.

@amerjusupovic amerjusupovic merged commit c1cdea3 into main Mar 12, 2025
3 checks passed
@amerjusupovic amerjusupovic deleted the user/jimmyca/kvCollectionRefreshCheck branch March 12, 2025 20:47
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