-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial support for testing #9
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,6 +75,7 @@ public class PodManager { | |
* @param keystorePath Path to the Java Keystore containing trusted certificates | ||
* @param maximumPodCount Maximum number of pods to ever have running at once | ||
* @param refreshIntervalInMs Interval with which to refresh the pods | ||
* @param podYamlPath the location of the yaml config for the application pod | ||
*/ | ||
public PodManager(KubernetesConfiguration k8sConfiguration, | ||
String keystorePath, | ||
|
@@ -343,7 +344,7 @@ else if (left.getLastRequest() < right.getLastRequest()) | |
* @throws IOException | ||
*/ | ||
private void refreshPods() throws IOException { | ||
HttpGet podsGet = new HttpGet("https://" + k8sConfiguration.getMasterIp() + ":443/api/v1/namespaces/" + k8sConfiguration.getNamespace() + "/pods"); | ||
HttpGet podsGet = new HttpGet(k8sConfiguration.getProtocol() + "://" + k8sConfiguration.getMasterIp() + "/api/v1/namespaces/" + k8sConfiguration.getNamespace() + "/pods"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could drop the protocol and master ip and just have a URL. master_url: https://k8s.example.com
# or
master_url: http://k8s.example.com I'm fairly certain I just went with master IP since that is what the GKE console displayed. It's fairly straightforward to reason out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like having an optional protocol for that reason. It's less to think about when you are configuring it. In almost all cases (except test) we can assume it's https. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to see the GKE dashboard again and verify they just surface up an IP. If we rename the parameter to |
||
|
||
try (CloseableHttpResponse response = httpClient.execute(podsGet, httpContext)) { | ||
HttpEntity entity = response.getEntity(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
package com.o19s.grandcentral.kubernetes; | ||
import com.github.tomakehurst.wiremock.junit.WireMockRule; | ||
import org.junit.After; | ||
import org.junit.Before; | ||
import org.junit.Rule; | ||
import org.junit.Test; | ||
|
||
import static com.github.tomakehurst.wiremock.client.WireMock.*; | ||
|
||
; | ||
/** | ||
* Created by Omnifroodle on 5/17/16. | ||
*/ | ||
public class PodManagerTest { | ||
private KubernetesConfiguration kubecfg; | ||
@Rule | ||
public WireMockRule wireMockRule = new WireMockRule(8888); | ||
|
||
@Before | ||
public void setUp() throws Exception { | ||
this.kubecfg = new KubernetesConfiguration(); | ||
kubecfg.setMasterIp("127.0.0.1:8888"); | ||
kubecfg.setProtocol("http"); | ||
kubecfg.setNamespace("test"); | ||
kubecfg.setUsername("fred"); | ||
kubecfg.setPassword("flintstone"); | ||
} | ||
|
||
@After | ||
public void tearDown() throws Exception { | ||
} | ||
|
||
@Test | ||
public void testGet() throws Exception { | ||
|
||
} | ||
|
||
@Test | ||
public void testContains() throws Exception { | ||
|
||
} | ||
|
||
@Test | ||
public void testAdd() throws Exception { | ||
|
||
} | ||
|
||
@Test | ||
public void testRefreshPods() throws Exception { | ||
stubFor(get(urlEqualTo("/api/v1/namespaces/test/pods")) | ||
.willReturn(aResponse() | ||
.withStatus(200) | ||
.withHeader("Content-Type", "text/json") | ||
.withBody("{\"items\":[" + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be possible to have this stored in a JSON file on disk then consumed by the test? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think yes, I was actually looking into that last night. I'm trying to decide which is more readable, ugly multiline java string literals or a set of hidden away response files There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see it as being hidden away the full path will be here in the test. I believe that's a fairly common pattern to keep tests readable / concise. It is slightly inconvenient to track down the file when it needs to be changed, but I prefer the cleaner test. |
||
" {" + | ||
" \"metadata\":{" + | ||
" \"name\": \"abc\"" + | ||
" }," + | ||
" \"status\": {" + | ||
" \"phase\": \"Running\"," + | ||
" \"podIP\": \"1.1.1.1\"" + | ||
" }" + | ||
" }," + | ||
" {" + | ||
" \"metadata\":{" + | ||
" \"name\": \"abcd\"" + | ||
" }," + | ||
" \"status\": {" + | ||
" \"phase\": \"Running\"," + | ||
" \"podIP\": \"1.1.1.2\"" + | ||
" }" + | ||
" }" + | ||
" ]}"))); | ||
|
||
PodManager manager = new PodManager(this.kubecfg, "config/grandcentral.jks", 100, 1, "./config/configuration.yml"); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not put this version number with the others in
properties
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no reason, I'll fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I go back and forth. If it’s version used by a number of jar files that are related, then I like using the interpolation. If it’s a single jar, then it’s yet another level of indirection.
Regardless, good to see…
Eric Pugh | Principal | OpenSource Connections, LLC | 434.466.1467 | http://www.opensourceconnections.com http://www.opensourceconnections.com/ | My Free/Busy http://tinyurl.com/eric-cal
Co-Author: Apache Solr Enterprise Search Server, 3rd Ed https://www.packtpub.com/big-data-and-business-intelligence/apache-solr-enterprise-search-server-third-edition-raw
This e-mail and all contents, including attachments, is considered to be Company Confidential unless explicitly stated otherwise, regardless of whether attachments are marked as such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point on level of indirection, but it is convenient to have all version numbers together. If we include them with each dependency then you're crawling through each dependency's XML block a) looking for the right block and b) looking for the version element.
Fortunately our number of dependencies is small so it isn't that big of a task. I'm open for either approach. @epugh has a point.