adding custom ConfigBuilder by karthikkondapally · Pull Request #171 · kubernetes-client/java

karthikkondapally · 2018-01-22T18:02:41Z

custom ConfigBuilder. issue: #168

karthikkondapally · 2018-01-22T18:21:28Z

sorry, will correct unit tests...

brendandburns · 2018-01-24T05:45:11Z

+		return this;
+	}
+
+	public String getbasePath() {


getBasePath()

brendandburns · 2018-01-24T05:45:36Z

+		return basePath;
+	}
+
+	public ConfigBuilder setbasePath(String basePath) {


setBasePath(...)

brendandburns · 2018-01-24T05:46:27Z

+	}
+
+	public ApiClient build()  {
+


Delete newline

brendandburns · 2018-01-24T05:49:15Z

+
+		if( kubeConfig !=null) {
+			client = Config.fromConfig(kubeConfig);
+			return client;


I think this logic is a little odd.

I would expect that I could do:

client = new ConfigBuilder().setKubeConfig().setBasePath().build()

And it would mostly load the KubeConfig, but then override the basepath with the custom value I set.

Instead, it just returns the existing KubeConfig and ignores any other setFoo directives.

I think that this is going to confuse people, and doesn't match the desired use case.

brendandburns · 2018-01-24T05:49:27Z

+			} catch (FileNotFoundException e) {
+				e.printStackTrace();
+			}
+			return client;


brendandburns · 2018-01-24T05:49:37Z

+			} catch (IOException e) {
+				e.printStackTrace();
+			}
+			return client;


brendandburns · 2018-01-24T05:49:55Z

+				e.printStackTrace();
+			}
+			return client;
+


delete newline (here and everywhere it's extraneous)

brendandburns · 2018-01-24T05:50:21Z

+				basePath = basePath.substring(0, basePath.length() - 1);
+			}
+			client.setBasePath(basePath)
+			.setVerifyingSsl(verifyingSsl);


why are you setting verifyingSsl here?

by default(if user doesn't set anything ), want to set verifyingSsl to false.
but i forget to add the actual setter..(will rectify it)
if( verifyingSsl != null ){
client..setVerifyingSsl(verifyingSsl);
}

brendandburns · 2018-01-24T05:51:17Z

+		}
+
+		else {
+			try {


This is weird. You are try{ ... } catch and then throwing the exception yourself. Just log to stderr? or throw an exception?

brendandburns · 2018-01-24T05:53:17Z

+			if((certificateAuthorityData != null) || (certificateAuthorityFile != null)){
+				try {
+					client.setSslCaCert(SSLUtils.getInputStreamFromDataOrFile(certificateAuthorityData, certificateAuthorityFile));
+				} catch (FileNotFoundException e) {


I think you should load the cert when someone calls setCertificateAuthority(...)

And not throw/catch here.

brendandburns · 2018-01-24T05:54:01Z

+		return this;
+	}
+
+	public boolean isTrustCerts() {


This doesn't seem to do anything. Delete?

brendandburns · 2018-01-26T05:50:06Z

+		return this;
+	}
+
+	public ConfigBuilder setKubeConfig(String fileName) throws FileNotFoundException {


It's a little weird to use File for CertificateAuthority and String here, I think consistency one way or the other is probably better.

brendandburns · 2018-01-26T05:51:22Z

+		}
+
+		if(defaultKubeConfigMode == true) {
+			try {


~~Let's get rid of the try/catch and just have build() throw, I think that's better.~~

Better: move KubeConfig.loadDefaultKubeConfig() up into the setDefaultMode() function.

@brendandburns
is the following what you intend

public ConfigBuilder setDefaultMode() { this.defaultMode = true; return this; } build() throws Exception{ .... if(defaultMode == true) { client = Config.fromConfig(KubeConfig.loadDefaultKubeConfig()); } ... }

@kondapally1989 no, what I was thinking was this:

public ConfigBuilder setDefaultMode() throws IOException { this.client = config.fromConfig(KubeConfig.loadDefaultKubeConfig()); }

@brendandburns
if setDefaultMode() points to Config.fromConfig(KubeConfig.loadDefaultKubeConfig())

and setDefaultClientMode points to Config.defaultClient();

isn't the naming convention confusing. instead of setDefaultMode() cant we have setDefaultKubeConfigMode()

brendandburns · 2018-01-26T05:52:58Z

+
+		if(clusterMode == true) {
+			try {
+				client = Config.fromCluster();


brendandburns · 2018-01-26T05:53:22Z

+
+		if(defaultClientMode ==true ) {
+			try {
+				client = Config.defaultClient();


brendandburns · 2018-01-26T05:54:17Z

+			}
+			client.setBasePath(basePath);
+		} else {
+			if((clusterMode == false) && (defaultClientMode == false) && (defaultKubeConfigMode == false)) {


I wouldn't throw here, I would just do:

if (client.getBasePath().length() == 0) { client.setBasePath("http://localhost:8080") }

brendandburns · 2018-01-26T05:55:11Z

+
+		if(certificateAuthorityFile != null) {
+			try {
+				client.setSslCaCert(new FileInputStream(certificateAuthorityFile));


Let's move this exception up to the setCertificateAuthority call...

brendandburns · 2018-01-26T05:55:42Z

A few more comments, thanks for your patience, this is getting close!

lewisheadden · 2018-01-27T18:11:16Z

This look great!

An oddity about Config which has bothered me is that it returns ApiClient not configurations and I was wondering whether this should really be called ClientBuilder or similar given that it returns an ApiClient?

lewisheadden · 2018-01-27T18:40:26Z

Also any thoughts on doing something like lewisheadden@ac7c1b5? It seems like it's hard to know which set of fields should be set for which auth method and this might simplify it.

karthikkondapally · 2018-01-28T10:52:04Z

ClientBuilder sounds apt.
and
lewisheadden@ac7c1b5
@brendanburns ?

brendandburns · 2018-02-01T00:07:37Z

That approach seems reasonable...

karthikkondapally · 2018-02-01T06:11:28Z

ok i will change it to ClientBuilder.
@lewisheadden is it okay if I push the changes you suggested or will you push after this is completed ?

lewisheadden · 2018-02-01T16:44:40Z

@kondapally1989 I can open a PR afterwards if that's easier for you. The example I pushed needs cleaned up.

karthikkondapally · 2018-02-02T18:16:30Z

@lewisheadden just changed the classname and few changes, you can create PR for the changes you suggested after this PR is done.

brendandburns · 2018-02-02T18:35:54Z

LGTM, thanks for the patience. There are a couple more cleanups, but they can wait for a follow-on PR.

brendandburns · 2018-02-02T18:36:35Z

+		return this;
+	}
+
+	public ApiClient build() throws FileNotFoundException {


I'd like to see this get to a point where it doesn't throw FileNotFoundException I think that means moving the cert loading to the setter function.

adding custom ConfigBuilder

58f364b

fix ConfigBuilder unit tests

412d8af

brendandburns reviewed Jan 24, 2018

View reviewed changes

review changes

bf764a4

brendandburns reviewed Jan 26, 2018

View reviewed changes

review changes and ConfigBuilder to ClientBuilder name change

2428033

brendandburns merged commit 3b9000b into kubernetes-client:master Feb 2, 2018

brendandburns reviewed Feb 2, 2018

View reviewed changes

+              		}
+              		else {
+              			try {

Conversation

karthikkondapally commented Jan 22, 2018

Uh oh!

karthikkondapally commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brendandburns Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendandburns Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendandburns commented Jan 26, 2018

Uh oh!

lewisheadden commented Jan 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lewisheadden commented Jan 27, 2018

Uh oh!

karthikkondapally commented Jan 28, 2018

Uh oh!

brendandburns commented Feb 1, 2018

Uh oh!

karthikkondapally commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lewisheadden commented Feb 1, 2018

Uh oh!

karthikkondapally commented Feb 2, 2018

Uh oh!

brendandburns commented Feb 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

karthikkondapally commented Jan 22, 2018 •

edited

Loading

brendandburns Jan 24, 2018 •

edited

Loading

brendandburns Jan 26, 2018 •

edited

Loading

lewisheadden commented Jan 27, 2018 •

edited

Loading

karthikkondapally commented Feb 1, 2018 •

edited

Loading