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.

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

Add Eager Options Validation: ValidateOnStart API #47821

Merged
merged 9 commits into from Feb 17, 2021
Merged

Add Eager Options Validation: ValidateOnStart API #47821

merged 9 commits into from Feb 17, 2021

Conversation

maryamariyan
Copy link
Member

@maryamariyan maryamariyan commented Feb 3, 2021 •
edited

  • Supports Options Validation, getting triggered at startup via a hosted service.
  • Allows options validation for named options as well.

Note: Microsoft.Extensions.Options would need a new dependency to Microsoft.Extensions.Hosting.Abstractions because we use IHostedService to implement this feature.

Minor TODO:

  • Improve unit tests + code coverage
  • Get API approved
  • Cleanup comments

Fixes #36391

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Feb 3, 2021

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Supports Options Validation, getting triggered at startup via a hosted service.
  • Allows options validation for named options as well.

Note: Microsoft.Extensions.Options would need a new dependency to Microsoft.Extensions.Hosting.Abstractions because we use IHostedService to implement this feature.

Minor TODO:

  • Improve unit tests
  • Get approval on .NET API review meeting tomorrow on:
    (1) Namespace for API (Was originally approved to go under Microsoft.Extensions.Options.DataAnnotations but that is not needed, since the feature is not tied to Data Annotations so it is best to be part of Microsoft.Extensions.Options).
    (2) Finalize name ValidateOnStart (rather than ValidateOnStartup)
Author: maryamariyan
Assignees: -
Labels:

area-Extensions-Options, new-api-needs-documentation
Milestone: -

@ghost ghost added this to Active PRs in ML, Extensions, Globalization, etc, POD. Feb 3, 2021
@maryamariyan
Apply PR feedback:
2d6a5d9 
- Drop new() constraint
- Remove _ in most cases
- Remove the ConcurrentDictionary
- Slight comment cleanup
- Skip tests on mono: due to PNSE on host.StartAsync()
@maryamariyan
Cleanup test class
68e1b66
@maryamariyan maryamariyan marked this pull request as ready for review February 5, 2021 03:40
@runfoapp runfoapp bot mentioned this pull request Feb 5, 2021
Closed
@runfoapp runfoapp bot mentioned this pull request Feb 8, 2021
Closed
@maryamariyan
Move to hosting assembly
8effbfc
@maryamariyan
- Locate tests properly:
9107eab 
Keeps DataAnnotations tests in Options and rest in Hosting
@maryamariyan
Copy link
Member Author

The CI failure is unrelated.

@davidfowl @eerhardt The PR should be ready for another review.

eerhardt
eerhardt approved these changes Feb 16, 2021
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for driving this through, @maryamariyan.

davidfowl
davidfowl approved these changes Feb 16, 2021
View reviewed changes
Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

Still want to discuss the home

@maryamariyan
Copy link
Member Author

maryamariyan commented Feb 16, 2021 •
edited

Still want to discuss the home

OK, marking unresolved the conversation thread above: #47821 (comment)

Here's a summary of the two approaches tried:

  • Microsoft.Extensions.Options (would need a reference to Microsoft.Extensions.Hosting.Abstractions to access IHostedService for implementing ValidateOnStart
  • Microsoft.Extensions.Hosting (in PR as-is, it already references M.E.Options and M.E.Hosting.Abstractions)

@maryamariyan maryamariyan merged commit bec81bd into dotnet:master Feb 17, 2021
ML, Extensions, Globalization, etc, POD. automation moved this from Active PRs to Done Feb 17, 2021
@dotnet dotnet locked as resolved and limited conversation to collaborators Mar 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@halter73 halter73 halter73 approved these changes

@davidfowl davidfowl davidfowl approved these changes

@eerhardt eerhardt eerhardt approved these changes

@safern safern Awaiting requested review from safern

Assignees
No one assigned
Labels
area-Extensions-Options new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Developers can get immediate feedback on validation problems
4 participants
@maryamariyan @halter73 @davidfowl @eerhardt