Make LoadKubeConfig public and refactor to use YAML helper methods #133
Conversation
billpratt changed the title 
| /// Gets or sets the path to a cert file for the certificate authority. | ||
| /// </summary> | ||
| [YamlMember(Alias = "certificate-authority")] | ||
| [YamlMember(Alias = "certificate-authority", ApplyNamingConventions = false)] |
There was a problem hiding this comment.
ApplyNamingConventions = false is need when using naming conventions like CamelCaseNamingConvention
| /// <param name="kubeconfig">String representation of kubeconfig contents, cannot be null, whitespaced or empty</param> | ||
| /// <param name="currentContext">override the context in config file, set null if do not want to override</param> | ||
| /// <param name="masterUrl">overrider kube api server endpoint, set null if do not want to override</param> | ||
| public static KubernetesClientConfiguration BuildConfigFromConfigFile(string kubeconfig, |
There was a problem hiding this comment.
I couldn't find any references to this method. Can we deprecate?
There was a problem hiding this comment.
Sure, seems reasonable.
There was a problem hiding this comment.
Mark as [Obsolete] or remove?
| /// <param name="kubeconfig">Kube config file contents</param> | ||
| /// <returns>Instance of the <see cref="K8SConfiguration"/> class</returns> | ||
| private static K8SConfiguration LoadKubeConfig(Stream kubeconfig) | ||
| public static K8SConfiguration LoadKubeConfig(FileInfo kubeconfig) |
There was a problem hiding this comment.
Let's keep this as a stream, since we want to support loading from network streams as well as files.
There was a problem hiding this comment.
Good catch. I'll add it back in and use Yaml.LoadFromStreamAsync. Should we keep the method with FileInfo kubeconfig?
Essentially, we would have these method signatures.
public static async Task<K8SConfiguration> LoadKubeConfigAsync(string kubeconfigPath = null) public static K8SConfiguration LoadKubeConfig(string kubeconfigPath = null) public static async Task<K8SConfiguration> LoadKubeConfigAsync(FileInfo kubeconfig) public static K8SConfiguration LoadKubeConfig(FileInfo kubeconfig) public static async Task<K8SConfiguration> LoadKubeConfigAsync(Stream kubeconfigStream) public static K8SConfiguration LoadKubeConfig(Stream kubeconfigStream) There was a problem hiding this comment.
Your call about FileInfo. it's easy enough to turn a file name into a Stream
| ConfigsEqual(expectedCfg, cfg); | ||
| } | ||
| | ||
| private void ConfigsEqual(K8SConfiguration expected, K8SConfiguration actual) |
There was a problem hiding this comment.
This function is too long, I think. Factor it so there is an
assertContextEqual(...)
assertUserEqual(...)
assertClusterEqual(...)
set of methods and refactor the for loops to just call those methods?
| A few small comments, but basically LGTM. |
| We just merged a file move in #134 you'll need to rebase (but it should "just work") |
| @brendanburns I believe I've addressed everything. |
| LGTM, thanks! |
…ubernetes-client#133) * Made LoadKubeConfig public and refactored to use YAML helper methods * Addressing Code review feedback
2 participants
Addressed #120
@brendanburns With this PR I also refactored to use the YAML helper methods you added recently.