search

tintoy / dotnet-kube-client

A Kubernetes API client for .NET Standard / .NET Core
MIT License
192 stars 33 forks source link

Throw exception on missing Configmap? #88

Closed PhilippCh closed 5 years ago

PhilippCh commented 5 years ago

We've stumbled upon an issue where we didn't see that a configmap was not loaded correctly because of a typo and our logging level being set to Information+. It would be ideal if the library could throw an exception, seeing as missing ConfigMaps would be a major issue for most applications.

Would this be okay with you and if so, should we open a PR?

Referencing this line: https://github.com/tintoy/dotnet-kube-client/blob/e3d429ec9fcf185f6709613c32ad7846893522ba/src/KubeClient.Extensions.Configuration/ConfigMapConfigurationProvider.cs#L145

tintoy commented 5 years ago

Hi - if we could make this an opt-in behaviour I’d be quite happy with that :)

One of the things I’ve discovered building K8s-native apps is that you can’t always rely on the ConfigMap being available when your pod starts unless it directly references the ConfigMap in its spec (since much of K8s behaviour is asynchronous). Usually I prefer to have a readiness check that only passes once config is available, but your use case may be different of course.

Feel free to open a PR and I’m sure we can thrash out a solution between us :)

tintoy commented 5 years ago

This might be helpful:

https://github.com/aspnet/Extensions/blob/master/src/Configuration/Config.FileExtensions/src/FileConfigurationProvider.cs#L114

tintoy commented 5 years ago

Then again it might be overkill - essentially we need to throw on load, but not reload.

PhilippCh commented 5 years ago

I've submitted a short PR for a basic throw when no configmap was found during the initial load. If you'd prefer an opt-in variant, should we add it as a parameter to ConfigMapConfigurationProvider (and KubeClientConfigurationExtensions respectively)?

Also, seeing as no error is thrown by https://github.com/tintoy/dotnet-kube-client/blob/e3d429ec9fcf185f6709613c32ad7846893522ba/src/KubeClient.Extensions.Configuration/ConfigMapConfigurationProvider.cs#L108 when a configmap is missing, would you nevertheless regard this as an API exception or should we keep it as a client exception?

tintoy commented 5 years ago

Hi - thanks for doing that! If you wouldn’t mind, making it opt-in would probably be kinder to existing consumers who may not be expecting the change in behaviour :)