Get appsettings data from Kubernetes secrets #240
Conversation
…uduLite into svangala-zipdeploy
sushmithavangala
requested review from
ahmelsayed,
JatinSanghvi,
lpapudippu,
sanchitmehta and
TsuyoshiUshio
| @@ -76,17 +79,17 @@ bool IsHostJson(string fullName) | |||
| } | |||
|
|
|||
| internal static void UpdateFunctionTriggerBindingExpression( | |||
| IEnumerable<ScaleTrigger> scaleTriggers, IDictionary<string, string> appSettings) | |||
| IEnumerable<ScaleTrigger> scaleTriggers, IDictionary<string, byte[]> appSettings) | |||
There was a problem hiding this comment.
I'm curious why you change it from string to byte[]. Could you share the context?
There was a problem hiding this comment.
The kubernetes api gives secret data as IDictionary<string, byte[]> , so the same is passed ahead. There is also kubernetesSecret.stringData available in the output that should give secret as IDictionary<string, string> , but in testing we found kubernetesSecret.stringData is giving an empty dictionary , so we have proceeded with kubernetesSecret.Data which returns IDictionary<string, byte[]>
Kudu.Core/Environment.cs
Outdated
| @@ -47,6 +47,12 @@ public class Environment : IEnvironment | |||
|
|
|||
|
|
|||
| // This ctor is used only in unit tests | |||
|
|
|||
| // added for test cases | |||
| public Environment() | |||
There was a problem hiding this comment.
We can use Moq with IEnvironment for the testability.
| { | ||
| var config = KubernetesClientConfiguration.InClusterConfig(); | ||
| // Use the config object to create a client. | ||
| var client = new Kubernetes(config); |
There was a problem hiding this comment.
Generally speaking, instantiate HTTP Client every time is not good idea. https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclient?view=net-6.0 Kubernetes. We might pass the Http client for the object.
In this case, we can't use HttpClientFactory so that having it as static might be a good idea.
There was a problem hiding this comment.
@SushmithaVReddy Can you address this feedback? I think it's important.
|
Abandoning this PR as #242 fixes the same |
Fix for #231