Skip to content

Adjusts the configuration provider to no longer check top level for feature flags #261

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 29, 2023
Merged

Adjusts the configuration provider to no longer check top level for feature flags #261

merged 4 commits into from
Sep 29, 2023

Conversation

rossgrambo
Copy link
Contributor

With the release of v3 on the horizon, we can start adding misc. features that were waiting for a major version bump.

This adjustment was requested in a couple issues
#144
#254

@rossgrambo rossgrambo force-pushed the rossgrambo/provider-section-scope branch from cb43b80 to 74ee097 Compare September 13, 2023 23:11
@zhiyuanliang-ms zhiyuanliang-ms mentioned this pull request Sep 14, 2023
Closed
@rossgrambo rossgrambo changed the base branch from main to preview September 14, 2023 23:51
@rossgrambo rossgrambo force-pushed the rossgrambo/provider-section-scope branch 3 times, most recently from d48629c to 65e8330 Compare September 15, 2023 00:21
@rossgrambo rossgrambo changed the base branch from preview to main September 21, 2023 16:21
@rossgrambo
Copy link
Contributor Author

Now pointing this PR at main for a new major version.

@jimmyca15
Copy link
Member

The PR seems messed up. There is a huge change set.

@rossgrambo rossgrambo force-pushed the rossgrambo/provider-section-scope branch from 65e8330 to b91bc3e Compare September 22, 2023 00:38
@rossgrambo rossgrambo force-pushed the rossgrambo/provider-section-scope branch from b91bc3e to 85b401e Compare September 22, 2023 00:39
@rossgrambo
Copy link
Contributor Author

My fault. Didn't push up the rebase. Should be good now.

jimmyca15
jimmyca15 reviewed Sep 22, 2023
View reviewed changes
src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs Outdated
}
else
{
return _configuration.GetChildren();
_logger.LogWarning($"No configuration section named '{FeatureManagementSectionName}' was found.");
Copy link
Member

@jimmyca15 jimmyca15 Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

There's just no features defined. That's fine. We have a separate flag to throw or not if a requested feature is not found, so that's where any error indication would be applicable I would say.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For developers, this code gets hit on each IsEnabledAsync call and on direct calls to GetFeatureDefinitionAsync.

For IsEnabledAsync, we do log that the feature wasn't found. I'd be okay to take it out, but I think there is some value in indicating that it never found a FeatureManagement section at all.

But for direct calls to GetFeatureDefinitionAsync, this is the only warning. I suppose the resulting object being empty is indication that something is wrong.

What if we kept it but adjusted it to LogInformation?

Copy link
Member

@jimmyca15 jimmyca15 Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

LogDebug or LogTrace perhaps. This isn't really valuable information for the application.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusted to LogDebug

jimmyca15
jimmyca15 approved these changes Sep 29, 2023 •
edited
Loading
View reviewed changes
@rossgrambo rossgrambo merged commit 0eb3de9 into main Sep 29, 2023
rossgrambo added a commit that referenced this pull request Oct 5, 2023
@rossgrambo
Merge main changes into preview (#271)
3e9179a 
* Adjusts the configuration provider to no longer check top level for feature flags (#261)

* Adjusts the configuration provider to no longer check top level for feature flags

* Adding missing using and removes unused else

* Remove unused string

* Add null check to constructor

* Updates dependencies. Removes ASP.NET support for netstandard2.0 & tests for netcoreapp2.1 (#267)

* Updating dependencies

* Updates packages and resolves issues

* Remove pointless semicolon

* Adjusts dotnet installation for buiddy build

* Removes ASP.NET tests from .NET tests

* Removes unused usings

* Adjust the template type for RazorPages project

* Resolves missing ExternalInit for netstandard2.1
@FredericVaugeoisFlo FredericVaugeoisFlo mentioned this pull request Nov 10, 2023
Closed
@rossgrambo rossgrambo deleted the rossgrambo/provider-section-scope branch January 16, 2024 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amerjusupovic amerjusupovic amerjusupovic left review comments

@zhiyuanliang-ms zhiyuanliang-ms zhiyuanliang-ms left review comments

@jimmyca15 jimmyca15 jimmyca15 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rossgrambo @jimmyca15 @amerjusupovic @zhiyuanliang-ms