Automagic client creation #17
Conversation
|
Please rebase. |
|
@mbohlool rebased. Thanks! |
|
@mbohlool friendly ping on this one. Thanks! |
|
Look like this needs a rebase. |
| } | ||
| String homeDir = System.getenv("HOME"); | ||
| if (homeDir != null) { | ||
| File homeConfig = new File(new File(new File(homeDir), ".kube"), "config"); |
There was a problem hiding this comment.
Can we extract constant for all of these strings? ".kube", "config" "HOME", "KUBECONFIG". I suggest we use something like "~/.kube.config" in constant and expand ~.
| return fromCluster(); | ||
| } | ||
| ApiClient client = new ApiClient(); | ||
| client.setBasePath("http://localhost:8080"); |
There was a problem hiding this comment.
why this is not https? also better to have constant for this string too.
| environmentVariables.set("HOME", "/non-existent"); | ||
| try { | ||
| ApiClient client = Config.defaultClient(); | ||
| assertEquals(client.getBasePath(), "http://localhost:8080"); |
There was a problem hiding this comment.
can we test other cases too?
| <scope>test</scope> | ||
| </dependency> | ||
| <!-- https://mvnrepository.com/artifact/com.github.stefanbirkner/system-rules --> | ||
| <dependency> |
There was a problem hiding this comment.
I am not sure if this dependency is used!?
There was a problem hiding this comment.
The dependency provides the EnvironmentVariables class used here:
|
Comment addressed, please take another look. |
|
I still see unrelated commits in this PR. Can you please rebase and stash all commits into one? |
And some initial tests.
|
Rebased and squashed. Thanks! |
| if (currentContext == null) { | ||
| return; | ||
| System.err.println("Failed to find 'context' in " + ctx); |
| currentContext = (Map<String, Object>) findObject(contexts, context).get("context"); | ||
| Map<String, Object> ctx = findObject(contexts, context); | ||
| if (ctx == null) { | ||
| System.err.println("Failed to find context: " + context + " in " + contexts); |
There was a problem hiding this comment.
Should this be an exception?
There was a problem hiding this comment.
I debated this... I decided that I preferered "return false" rather than "throw" because the context not existing isn't "exceptional" per-se but rather a potentially expected outcome...
(and I should probably get rid of the stderr logging)
If you feel strongly about throwing instead of returning false, I'm ok with that, this is a gray area.
(there is some interesting discussion on the subject here: https://codereview.stackexchange.com/questions/11724/is-it-better-practice-to-have-void-method-throw-an-exception-or-to-have-the-meth)
There was a problem hiding this comment.
The stderr thing distracted me. I agree that returning false is better here. Can you get rid of the stderr logging?
|
I debated the question you raised, ultimately I decided to Thanks! |
|
As my only comment is the stderr things, to unblock other PRs, I am going to mark this as LGTM and merge it. You can remove those stderr logging in one of the other PRs if you see that fit. |
Fixes #12
Builds on top of #7, that PR should be merged first.