adding custom ConfigBuilder #171

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

util/src/main/java/io/kubernetes/client/util/ConfigBuilder.java

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


getBasePath()

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

util/src/main/java/io/kubernetes/client/util/ConfigBuilder.java

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


setBasePath(...)

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

util/src/main/java/io/kubernetes/client/util/ConfigBuilder.java

+	}
+
+	public ApiClient build()  {
+


Delete newline

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

util/src/main/java/io/kubernetes/client/util/ConfigBuilder.java

+
+		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

util/src/main/java/io/kubernetes/client/util/ConfigBuilder.java

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


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

util/src/main/java/io/kubernetes/client/util/ConfigBuilder.java

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


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

util/src/main/java/io/kubernetes/client/util/ConfigBuilder.java

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


delete newline (here and everywhere it's extraneous)

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

util/src/main/java/io/kubernetes/client/util/ConfigBuilder.java

+				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

util/src/main/java/io/kubernetes/client/util/ConfigBuilder.java

+		}
+
+		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

util/src/main/java/io/kubernetes/client/util/ConfigBuilder.java

+			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

util/src/main/java/io/kubernetes/client/util/ConfigBuilder.java

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


This doesn't seem to do anything. Delete?

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

util/src/main/java/io/kubernetes/client/util/ConfigBuilder.java

+		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

util/src/main/java/io/kubernetes/client/util/ConfigBuilder.java

+		}
+
+		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

util/src/main/java/io/kubernetes/client/util/ConfigBuilder.java

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


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

util/src/main/java/io/kubernetes/client/util/ConfigBuilder.java

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


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

util/src/main/java/io/kubernetes/client/util/ConfigBuilder.java

+			}
+			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

util/src/main/java/io/kubernetes/client/util/ConfigBuilder.java

+
+		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

util/src/main/java/io/kubernetes/client/util/ClientBuilder.java

+		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

util/src/main/java/io/kubernetes/client/util/ConfigBuilder.java Outdated

}

public ApiClient build() {

Copy link

Contributor

brendandburns Jan 24, 2018

Choose a reason for hiding this comment

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

Delete newline

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding custom ConfigBuilder #171

adding custom ConfigBuilder #171

karthikkondapally commented Jan 22, 2018

karthikkondapally commented Jan 22, 2018 •

edited

Loading

brendandburns Jan 24, 2018 •

edited

Loading

brendandburns Jan 24, 2018

brendandburns Jan 24, 2018

brendandburns Jan 24, 2018

brendandburns Jan 24, 2018

brendandburns Jan 24, 2018

brendandburns Jan 24, 2018

brendandburns Jan 24, 2018

karthikkondapally Jan 24, 2018

brendandburns Jan 24, 2018

brendandburns Jan 24, 2018

brendandburns Jan 24, 2018

brendandburns Jan 26, 2018

brendandburns Jan 26, 2018 •

edited

Loading

brendandburns Jan 26, 2018

karthikkondapally Jan 26, 2018

brendandburns Jan 27, 2018

karthikkondapally Jan 27, 2018

brendandburns Jan 26, 2018

brendandburns Jan 26, 2018

brendandburns Jan 26, 2018

brendandburns Jan 26, 2018

brendandburns commented Jan 26, 2018

lewisheadden commented Jan 27, 2018 •

edited

Loading

lewisheadden commented Jan 27, 2018

karthikkondapally commented Jan 28, 2018

brendandburns commented Feb 1, 2018

karthikkondapally commented Feb 1, 2018 •

edited

Loading

lewisheadden commented Feb 1, 2018

karthikkondapally commented Feb 2, 2018

brendandburns commented Feb 2, 2018

brendandburns Feb 2, 2018

+              		}
+              		else {
+              			try {

adding custom ConfigBuilder #171

adding custom ConfigBuilder #171

Conversation

karthikkondapally commented Jan 22, 2018

karthikkondapally commented Jan 22, 2018 • edited Loading

brendandburns Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandburns Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandburns commented Jan 26, 2018

lewisheadden commented Jan 27, 2018 • edited Loading

lewisheadden commented Jan 27, 2018

karthikkondapally commented Jan 28, 2018

brendandburns commented Feb 1, 2018

karthikkondapally commented Feb 1, 2018 • edited Loading

lewisheadden commented Feb 1, 2018

karthikkondapally commented Feb 2, 2018

brendandburns commented Feb 2, 2018

Choose a reason for hiding this comment

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