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

Make config fields nullable #596

Merged
merged 1 commit into from
Dec 8, 2020

Conversation

Kahbazi
Copy link
Collaborator

@Kahbazi Kahbazi commented Dec 3, 2020

Fixes #531

@Tratcher Tratcher self-assigned this Dec 7, 2020
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

I had meant for this to cover the config model types consumed through the IProxyConfigProvider (Cluster, ClusterProxyHttpClientOptions, etc.), I hadn't considered the IOptions<T> types like ActiveHealthCheckMonitorOptions. Thanks for being thorough 😁.

Having reviewed those IOptions types now, it looks like most of them were already intended to provide fallback defaults for fields from the config model. These types are also used directly by implementations, where an alternate implementation would likely provide its own options type. I think it's fine for these to keep their non-nullable default values. Please revert changes for these options and their consumers:

  • ActiveHealthCheckMonitorOptions
  • ConsecutiveFailuresHealthPolicyOptions
  • TransportFailureRateHealthPolicyOptions

@@ -8,7 +8,7 @@ namespace Microsoft.ReverseProxy.Abstractions
/// </summary>
public sealed class LoadBalancingOptions
{
public LoadBalancingMode Mode { get; set; }
public LoadBalancingMode? Mode { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

FYI: Conflicts with #600. We'll see which merges first.

@Tratcher Tratcher merged commit e4426b9 into microsoft:master Dec 8, 2020
@Tratcher
Copy link
Member

Tratcher commented Dec 8, 2020

Thanks

@Tratcher Tratcher added this to the YARP 1.0.0-preview8 milestone Dec 8, 2020
@Kahbazi Kahbazi deleted the kahbazi/nullableConfig branch December 8, 2020 17:28
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.

Inconsistent use of nullable fields in config
2 participants