From 2f9bafd36f772a9844cd0dee4c936f4a49778453 Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Wed, 18 Jul 2018 16:44:10 -0400 Subject: [PATCH 01/15] Enable jib.httpTimeout system property --- .../cloud/tools/jib/http/Connection.java | 2 + .../google/cloud/tools/jib/http/Request.java | 22 +++++++- .../jib/registry/RegistryAuthenticator.java | 7 ++- .../jib/registry/RegistryEndpointCaller.java | 6 +++ .../cloud/tools/jib/http/ConnectionTest.java | 13 +++++ .../cloud/tools/jib/http/MockConnection.java | 47 ++++++++++++++++ .../cloud/tools/jib/http/RequestTest.java | 31 +++++++++++ .../registry/RegistryEndpointCallerTest.java | 53 +++++++++++++++++++ 8 files changed, 179 insertions(+), 2 deletions(-) create mode 100644 jib-core/src/test/java/com/google/cloud/tools/jib/http/MockConnection.java create mode 100644 jib-core/src/test/java/com/google/cloud/tools/jib/http/RequestTest.java diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/http/Connection.java b/jib-core/src/main/java/com/google/cloud/tools/jib/http/Connection.java index 9df7dd6dd6..9f7e41775e 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/http/Connection.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/http/Connection.java @@ -121,6 +121,8 @@ public Response send(String httpMethod, Request request) throws IOException { requestFactory .buildRequest(httpMethod, url, request.getHttpContent()) .setHeaders(request.getHeaders()) + .setConnectTimeout(request.getHttpTimeout()) + .setReadTimeout(request.getHttpTimeout()) .execute(); return new Response(httpResponse); } diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/http/Request.java b/jib-core/src/main/java/com/google/cloud/tools/jib/http/Request.java index b3fae5cf59..5d68c616c2 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/http/Request.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/http/Request.java @@ -27,12 +27,16 @@ public class Request { private final HttpHeaders headers; /** The HTTP request body. */ - @Nullable private BlobHttpContent body; + @Nullable private final BlobHttpContent body; + + /** HTTP connection and read timeout. */ + private final int httpTimeout; public static class Builder { private final HttpHeaders headers = new HttpHeaders().setAccept("*/*"); @Nullable private BlobHttpContent body; + private int httpTimeout = 20000; // ms public Request build() { return new Request(this); @@ -73,6 +77,17 @@ public Builder setUserAgent(String userAgent) { return this; } + /** + * Sets the HTTP connection and read timeout in milliseconds. + * + * @param httpTimeout timeout in milliseconds + * @return this + */ + public Builder setHttpTimeout(int httpTimeout) { + this.httpTimeout = httpTimeout; + return this; + } + /** * Sets the body and its corresponding {@code Content-Type} header. * @@ -92,6 +107,7 @@ public static Builder builder() { private Request(Builder builder) { this.headers = builder.headers; this.body = builder.body; + this.httpTimeout = builder.httpTimeout; } HttpHeaders getHeaders() { @@ -102,4 +118,8 @@ HttpHeaders getHeaders() { BlobHttpContent getHttpContent() { return body; } + + int getHttpTimeout() { + return httpTimeout; + } } diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAuthenticator.java b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAuthenticator.java index 1fabdb6acb..9be585e1f2 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAuthenticator.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAuthenticator.java @@ -103,6 +103,11 @@ public static Initializer initializer(String serverUrl, String repository) { return new Initializer(serverUrl, repository); } + private static int getHttpTimeout() { + int httpTimeout = Integer.getInteger("jib.httpTimeout", 20000); + return httpTimeout >= 0 ? httpTimeout : 20000; + } + // TODO: Replace with a WWW-Authenticate header parser. /** * Instantiates from parsing a {@code WWW-Authenticate} header. @@ -239,7 +244,7 @@ private Authorization authenticate(String scope) throws RegistryAuthenticationFa URL authenticationUrl = getAuthenticationUrl(scope); try (Connection connection = new Connection(authenticationUrl)) { - Request.Builder requestBuilder = Request.builder(); + Request.Builder requestBuilder = Request.builder().setHttpTimeout(getHttpTimeout()); if (authorization != null) { requestBuilder.setAuthorization(authorization); } diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java index f59e042fd1..d404d95b7d 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java @@ -69,6 +69,11 @@ static class RequestState { } } + private static int getHttpTimeout() { + int httpTimeout = Integer.getInteger("jib.httpTimeout", 20000); + return httpTimeout >= 0 ? httpTimeout : 20000; + } + /** Makes a {@link Connection} to the specified {@link URL}. */ private final Function connectionFactory; @@ -162,6 +167,7 @@ T call(RequestState requestState) throws IOException, RegistryException { Request.Builder requestBuilder = Request.builder() .setUserAgent(userAgent) + .setHttpTimeout(getHttpTimeout()) .setAccept(registryEndpointProvider.getAccept()) .setBody(registryEndpointProvider.getContent()); // Only sends authorization if using HTTPS. diff --git a/jib-core/src/test/java/com/google/cloud/tools/jib/http/ConnectionTest.java b/jib-core/src/test/java/com/google/cloud/tools/jib/http/ConnectionTest.java index e6c5045d5d..f00662de8f 100644 --- a/jib-core/src/test/java/com/google/cloud/tools/jib/http/ConnectionTest.java +++ b/jib-core/src/test/java/com/google/cloud/tools/jib/http/ConnectionTest.java @@ -63,6 +63,7 @@ public void setUpMocksAndFakes() throws IOException { .setUserAgent("fake user agent") .setBody(new BlobHttpContent(Blobs.from("crepecake"), "fake.content.type")) .setAuthorization(Authorizations.withBasicCredentials("fake-username", "fake-secret")) + .setHttpTimeout(54982) .build(); Mockito.when( @@ -72,6 +73,8 @@ public void setUpMocksAndFakes() throws IOException { Mockito.when(mockHttpRequest.setHeaders(Mockito.any(HttpHeaders.class))) .thenReturn(mockHttpRequest); + Mockito.when(mockHttpRequest.setConnectTimeout(Mockito.anyInt())).thenReturn(mockHttpRequest); + Mockito.when(mockHttpRequest.setReadTimeout(Mockito.anyInt())).thenReturn(mockHttpRequest); Mockito.when(mockHttpRequest.execute()).thenReturn(mockHttpResponse); } @@ -90,6 +93,16 @@ public void testPut() throws IOException { testSend(HttpMethods.PUT, Connection::put); } + @Test + public void testHttpTimeout() throws IOException { + try (Connection connection = testConnection) { + connection.send(HttpMethods.GET, fakeRequest); + } + + Mockito.verify(mockHttpRequest).setConnectTimeout(54982); + Mockito.verify(mockHttpRequest).setReadTimeout(54982); + } + @FunctionalInterface private interface SendFunction { diff --git a/jib-core/src/test/java/com/google/cloud/tools/jib/http/MockConnection.java b/jib-core/src/test/java/com/google/cloud/tools/jib/http/MockConnection.java new file mode 100644 index 0000000000..376afae895 --- /dev/null +++ b/jib-core/src/test/java/com/google/cloud/tools/jib/http/MockConnection.java @@ -0,0 +1,47 @@ +/* + * Copyright 2018 Google LLC. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package com.google.cloud.tools.jib.http; + +import com.google.api.client.http.GenericUrl; +import java.io.IOException; +import java.util.function.BiFunction; + +/** + * Mock {@link Connection} used for testing. Normally, you would use {@link + * org.mockito.Mockito#mock}; this class is intended to examine the {@link Request) object by + * calling its non-public package-protected methods. + */ +public class MockConnection extends Connection { + + private BiFunction responseSupplier; + private int httpTimeout; + + public MockConnection(BiFunction responseSupplier) { + super(new GenericUrl("ftp://non-exisiting.example.url.ever").toURL()); + this.responseSupplier = responseSupplier; + } + + @Override + public Response send(String httpMethod, Request request) throws IOException { + httpTimeout = request.getHttpTimeout(); + return responseSupplier.apply(httpMethod, request); + } + + public int getRequestedHttpTimeout() { + return httpTimeout; + } +} diff --git a/jib-core/src/test/java/com/google/cloud/tools/jib/http/RequestTest.java b/jib-core/src/test/java/com/google/cloud/tools/jib/http/RequestTest.java new file mode 100644 index 0000000000..face73259a --- /dev/null +++ b/jib-core/src/test/java/com/google/cloud/tools/jib/http/RequestTest.java @@ -0,0 +1,31 @@ +/* + * Copyright 2018 Google LLC. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package com.google.cloud.tools.jib.http; + +import org.junit.Assert; +import org.junit.Test; + +/** Tests for {@link Request}. */ +public class RequestTest { + + @Test + public void testSetHttpTimeout() { + Request request = Request.builder().setHttpTimeout(3000).build(); + + Assert.assertEquals(3000, request.getHttpTimeout()); + } +} diff --git a/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java b/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java index e1c6f22c04..67e8b22088 100644 --- a/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java +++ b/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java @@ -20,10 +20,12 @@ import com.google.api.client.http.HttpResponse; import com.google.api.client.http.HttpResponseException; import com.google.api.client.http.HttpStatusCodes; +import com.google.cloud.tools.jib.blob.Blob; import com.google.cloud.tools.jib.blob.Blobs; import com.google.cloud.tools.jib.http.Authorizations; import com.google.cloud.tools.jib.http.BlobHttpContent; import com.google.cloud.tools.jib.http.Connection; +import com.google.cloud.tools.jib.http.MockConnection; import com.google.cloud.tools.jib.http.Response; import com.google.cloud.tools.jib.json.JsonTemplateMapper; import com.google.cloud.tools.jib.registry.RegistryEndpointCaller.RequestState; @@ -253,6 +255,57 @@ public void testCall_disallowInsecure() throws IOException, RegistryException { } } + @Test + public void testHttpTimeout_default20000() throws IOException, RegistryException { + Assert.assertNull(System.getProperty("jib.httpTimeout")); + MockConnection mockConnection = new MockConnection((httpMethod, request) -> mockResponse); + Mockito.when(mockConnectionFactory.apply(Mockito.any())).thenReturn(mockConnection); + + Mockito.when(mockResponse.getBody()).thenReturn(Mockito.mock(Blob.class)); + testRegistryEndpointCallerSecure.call(); + + Assert.assertEquals(20000, mockConnection.getRequestedHttpTimeout()); + } + + @Test + public void testHttpTimeout_stringValue() throws IOException, RegistryException { + System.setProperty("jib.httpTimeout", "random string"); + + MockConnection mockConnection = new MockConnection((httpMethod, request) -> mockResponse); + Mockito.when(mockConnectionFactory.apply(Mockito.any())).thenReturn(mockConnection); + + Mockito.when(mockResponse.getBody()).thenReturn(Mockito.mock(Blob.class)); + testRegistryEndpointCallerSecure.call(); + + Assert.assertEquals(20000, mockConnection.getRequestedHttpTimeout()); + } + + @Test + public void testHttpTimeout_negativeValue() throws IOException, RegistryException { + System.setProperty("jib.httpTimeout", "-1"); + + MockConnection mockConnection = new MockConnection((httpMethod, request) -> mockResponse); + Mockito.when(mockConnectionFactory.apply(Mockito.any())).thenReturn(mockConnection); + + Mockito.when(mockResponse.getBody()).thenReturn(Mockito.mock(Blob.class)); + testRegistryEndpointCallerSecure.call(); + + Assert.assertEquals(20000, mockConnection.getRequestedHttpTimeout()); + } + + @Test + public void testHttpTimeout_0accepted() throws IOException, RegistryException { + System.setProperty("jib.httpTimeout", "0"); + + MockConnection mockConnection = new MockConnection((httpMethod, request) -> mockResponse); + Mockito.when(mockConnectionFactory.apply(Mockito.any())).thenReturn(mockConnection); + + Mockito.when(mockResponse.getBody()).thenReturn(Mockito.mock(Blob.class)); + testRegistryEndpointCallerSecure.call(); + + Assert.assertEquals(0, mockConnection.getRequestedHttpTimeout()); + } + /** Verifies a request is retried with HTTP protocol if {@code exceptionClass} is thrown. */ private void verifyRetriesWithHttp(Class exceptionClass) throws IOException, RegistryException { From 4654fac0a93af533b308a69d70468aa1532e4e72 Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Wed, 18 Jul 2018 16:47:19 -0400 Subject: [PATCH 02/15] Format code --- .../jib/registry/RegistryEndpointCallerTest.java | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java b/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java index 67e8b22088..fe635a2b75 100644 --- a/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java +++ b/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java @@ -257,11 +257,11 @@ public void testCall_disallowInsecure() throws IOException, RegistryException { @Test public void testHttpTimeout_default20000() throws IOException, RegistryException { - Assert.assertNull(System.getProperty("jib.httpTimeout")); MockConnection mockConnection = new MockConnection((httpMethod, request) -> mockResponse); Mockito.when(mockConnectionFactory.apply(Mockito.any())).thenReturn(mockConnection); - Mockito.when(mockResponse.getBody()).thenReturn(Mockito.mock(Blob.class)); + + Assert.assertNull(System.getProperty("jib.httpTimeout")); testRegistryEndpointCallerSecure.call(); Assert.assertEquals(20000, mockConnection.getRequestedHttpTimeout()); @@ -269,12 +269,11 @@ public void testHttpTimeout_default20000() throws IOException, RegistryException @Test public void testHttpTimeout_stringValue() throws IOException, RegistryException { - System.setProperty("jib.httpTimeout", "random string"); - MockConnection mockConnection = new MockConnection((httpMethod, request) -> mockResponse); Mockito.when(mockConnectionFactory.apply(Mockito.any())).thenReturn(mockConnection); - Mockito.when(mockResponse.getBody()).thenReturn(Mockito.mock(Blob.class)); + + System.setProperty("jib.httpTimeout", "random string"); testRegistryEndpointCallerSecure.call(); Assert.assertEquals(20000, mockConnection.getRequestedHttpTimeout()); @@ -282,12 +281,11 @@ public void testHttpTimeout_stringValue() throws IOException, RegistryException @Test public void testHttpTimeout_negativeValue() throws IOException, RegistryException { - System.setProperty("jib.httpTimeout", "-1"); - MockConnection mockConnection = new MockConnection((httpMethod, request) -> mockResponse); Mockito.when(mockConnectionFactory.apply(Mockito.any())).thenReturn(mockConnection); - Mockito.when(mockResponse.getBody()).thenReturn(Mockito.mock(Blob.class)); + + System.setProperty("jib.httpTimeout", "-1"); testRegistryEndpointCallerSecure.call(); Assert.assertEquals(20000, mockConnection.getRequestedHttpTimeout()); From 0af259401cb2712d7bfa68db0917e74faf580375 Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Wed, 18 Jul 2018 18:05:09 -0400 Subject: [PATCH 03/15] Set timeout only when property is defined --- .../cloud/tools/jib/http/Connection.java | 14 ++-- .../google/cloud/tools/jib/http/Request.java | 9 +-- .../jib/registry/RegistryAuthenticator.java | 10 ++- .../jib/registry/RegistryEndpointCaller.java | 10 ++- .../cloud/tools/jib/http/ConnectionTest.java | 67 ++++++++++++------- .../cloud/tools/jib/http/MockConnection.java | 4 +- .../cloud/tools/jib/http/RequestTest.java | 9 ++- .../registry/RegistryEndpointCallerTest.java | 29 ++++++-- 8 files changed, 103 insertions(+), 49 deletions(-) diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/http/Connection.java b/jib-core/src/main/java/com/google/cloud/tools/jib/http/Connection.java index 9f7e41775e..7c0702b4e3 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/http/Connection.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/http/Connection.java @@ -18,6 +18,7 @@ import com.google.api.client.http.GenericUrl; import com.google.api.client.http.HttpMethods; +import com.google.api.client.http.HttpRequest; import com.google.api.client.http.HttpRequestFactory; import com.google.api.client.http.HttpResponse; import com.google.api.client.http.apache.ApacheHttpTransport; @@ -117,13 +118,16 @@ public Response put(Request request) throws IOException { * @throws IOException if building the HTTP request fails. */ public Response send(String httpMethod, Request request) throws IOException { - httpResponse = + HttpRequest httpRequest = requestFactory .buildRequest(httpMethod, url, request.getHttpContent()) - .setHeaders(request.getHeaders()) - .setConnectTimeout(request.getHttpTimeout()) - .setReadTimeout(request.getHttpTimeout()) - .execute(); + .setHeaders(request.getHeaders()); + if (request.getHttpTimeout() != null) { + httpRequest.setConnectTimeout(request.getHttpTimeout()); + httpRequest.setReadTimeout(request.getHttpTimeout()); + } + + httpResponse = httpRequest.execute(); return new Response(httpResponse); } } diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/http/Request.java b/jib-core/src/main/java/com/google/cloud/tools/jib/http/Request.java index 5d68c616c2..8a94d5a1f1 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/http/Request.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/http/Request.java @@ -30,13 +30,13 @@ public class Request { @Nullable private final BlobHttpContent body; /** HTTP connection and read timeout. */ - private final int httpTimeout; + @Nullable private final Integer httpTimeout; public static class Builder { private final HttpHeaders headers = new HttpHeaders().setAccept("*/*"); @Nullable private BlobHttpContent body; - private int httpTimeout = 20000; // ms + @Nullable private Integer httpTimeout; public Request build() { return new Request(this); @@ -83,7 +83,7 @@ public Builder setUserAgent(String userAgent) { * @param httpTimeout timeout in milliseconds * @return this */ - public Builder setHttpTimeout(int httpTimeout) { + public Builder setHttpTimeout(@Nullable Integer httpTimeout) { this.httpTimeout = httpTimeout; return this; } @@ -119,7 +119,8 @@ BlobHttpContent getHttpContent() { return body; } - int getHttpTimeout() { + @Nullable + Integer getHttpTimeout() { return httpTimeout; } } diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAuthenticator.java b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAuthenticator.java index 9be585e1f2..0e4c2f941f 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAuthenticator.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAuthenticator.java @@ -103,9 +103,13 @@ public static Initializer initializer(String serverUrl, String repository) { return new Initializer(serverUrl, repository); } - private static int getHttpTimeout() { - int httpTimeout = Integer.getInteger("jib.httpTimeout", 20000); - return httpTimeout >= 0 ? httpTimeout : 20000; + @Nullable + private static Integer getHttpTimeout() { + Integer httpTimeout = Integer.getInteger("jib.httpTimeout"); + if (httpTimeout != null && httpTimeout >= 0) { + return httpTimeout; + } + return null; } // TODO: Replace with a WWW-Authenticate header parser. diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java index d404d95b7d..a43aa354df 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java @@ -69,9 +69,13 @@ static class RequestState { } } - private static int getHttpTimeout() { - int httpTimeout = Integer.getInteger("jib.httpTimeout", 20000); - return httpTimeout >= 0 ? httpTimeout : 20000; + @Nullable + private static Integer getHttpTimeout() { + Integer httpTimeout = Integer.getInteger("jib.httpTimeout"); + if (httpTimeout != null && httpTimeout >= 0) { + return httpTimeout; + } + return null; } /** Makes a {@link Connection} to the specified {@link URL}. */ diff --git a/jib-core/src/test/java/com/google/cloud/tools/jib/http/ConnectionTest.java b/jib-core/src/test/java/com/google/cloud/tools/jib/http/ConnectionTest.java index f00662de8f..a1a2152e5a 100644 --- a/jib-core/src/test/java/com/google/cloud/tools/jib/http/ConnectionTest.java +++ b/jib-core/src/test/java/com/google/cloud/tools/jib/http/ConnectionTest.java @@ -28,7 +28,6 @@ import java.nio.charset.StandardCharsets; import java.util.Arrays; import org.junit.Assert; -import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; @@ -55,52 +54,44 @@ public class ConnectionTest { @InjectMocks private final Connection testConnection = new Connection(fakeUrl.toURL()); - @Before - public void setUpMocksAndFakes() throws IOException { - fakeRequest = - Request.builder() - .setAccept(Arrays.asList("fake.accept", "another.fake.accept")) - .setUserAgent("fake user agent") - .setBody(new BlobHttpContent(Blobs.from("crepecake"), "fake.content.type")) - .setAuthorization(Authorizations.withBasicCredentials("fake-username", "fake-secret")) - .setHttpTimeout(54982) - .build(); - - Mockito.when( - mockHttpRequestFactory.buildRequest( - Mockito.any(String.class), Mockito.eq(fakeUrl), Mockito.any(BlobHttpContent.class))) - .thenReturn(mockHttpRequest); - - Mockito.when(mockHttpRequest.setHeaders(Mockito.any(HttpHeaders.class))) - .thenReturn(mockHttpRequest); - Mockito.when(mockHttpRequest.setConnectTimeout(Mockito.anyInt())).thenReturn(mockHttpRequest); - Mockito.when(mockHttpRequest.setReadTimeout(Mockito.anyInt())).thenReturn(mockHttpRequest); - Mockito.when(mockHttpRequest.execute()).thenReturn(mockHttpResponse); - } - @Test public void testGet() throws IOException { + setUpMocksAndFakes(null); testSend(HttpMethods.GET, Connection::get); } @Test public void testPost() throws IOException { + setUpMocksAndFakes(null); testSend(HttpMethods.POST, Connection::post); } @Test public void testPut() throws IOException { + setUpMocksAndFakes(null); testSend(HttpMethods.PUT, Connection::put); } + @Test + public void testHttpTimeout_doNotSetByDefault() throws IOException { + setUpMocksAndFakes(null); + try (Connection connection = testConnection) { + connection.send(HttpMethods.GET, fakeRequest); + } + + Mockito.verify(mockHttpRequest, Mockito.never()).setConnectTimeout(Mockito.anyInt()); + Mockito.verify(mockHttpRequest, Mockito.never()).setReadTimeout(Mockito.anyInt()); + } + @Test public void testHttpTimeout() throws IOException { + setUpMocksAndFakes(5982); try (Connection connection = testConnection) { connection.send(HttpMethods.GET, fakeRequest); } - Mockito.verify(mockHttpRequest).setConnectTimeout(54982); - Mockito.verify(mockHttpRequest).setReadTimeout(54982); + Mockito.verify(mockHttpRequest).setConnectTimeout(5982); + Mockito.verify(mockHttpRequest).setReadTimeout(5982); } @FunctionalInterface @@ -109,6 +100,30 @@ private interface SendFunction { Response send(Connection connection, Request request) throws IOException; } + private void setUpMocksAndFakes(Integer httpTimeout) throws IOException { + fakeRequest = + Request.builder() + .setAccept(Arrays.asList("fake.accept", "another.fake.accept")) + .setUserAgent("fake user agent") + .setBody(new BlobHttpContent(Blobs.from("crepecake"), "fake.content.type")) + .setAuthorization(Authorizations.withBasicCredentials("fake-username", "fake-secret")) + .setHttpTimeout(httpTimeout) + .build(); + + Mockito.when( + mockHttpRequestFactory.buildRequest( + Mockito.any(String.class), Mockito.eq(fakeUrl), Mockito.any(BlobHttpContent.class))) + .thenReturn(mockHttpRequest); + + Mockito.when(mockHttpRequest.setHeaders(Mockito.any(HttpHeaders.class))) + .thenReturn(mockHttpRequest); + if (httpTimeout != null) { + Mockito.when(mockHttpRequest.setConnectTimeout(Mockito.anyInt())).thenReturn(mockHttpRequest); + Mockito.when(mockHttpRequest.setReadTimeout(Mockito.anyInt())).thenReturn(mockHttpRequest); + } + Mockito.when(mockHttpRequest.execute()).thenReturn(mockHttpResponse); + } + private void testSend(String httpMethod, SendFunction sendFunction) throws IOException { try (Connection connection = testConnection) { sendFunction.send(connection, fakeRequest); diff --git a/jib-core/src/test/java/com/google/cloud/tools/jib/http/MockConnection.java b/jib-core/src/test/java/com/google/cloud/tools/jib/http/MockConnection.java index 376afae895..4e0e2611aa 100644 --- a/jib-core/src/test/java/com/google/cloud/tools/jib/http/MockConnection.java +++ b/jib-core/src/test/java/com/google/cloud/tools/jib/http/MockConnection.java @@ -28,7 +28,7 @@ public class MockConnection extends Connection { private BiFunction responseSupplier; - private int httpTimeout; + private Integer httpTimeout; public MockConnection(BiFunction responseSupplier) { super(new GenericUrl("ftp://non-exisiting.example.url.ever").toURL()); @@ -41,7 +41,7 @@ public Response send(String httpMethod, Request request) throws IOException { return responseSupplier.apply(httpMethod, request); } - public int getRequestedHttpTimeout() { + public Integer getRequestedHttpTimeout() { return httpTimeout; } } diff --git a/jib-core/src/test/java/com/google/cloud/tools/jib/http/RequestTest.java b/jib-core/src/test/java/com/google/cloud/tools/jib/http/RequestTest.java index face73259a..d142022d23 100644 --- a/jib-core/src/test/java/com/google/cloud/tools/jib/http/RequestTest.java +++ b/jib-core/src/test/java/com/google/cloud/tools/jib/http/RequestTest.java @@ -22,10 +22,17 @@ /** Tests for {@link Request}. */ public class RequestTest { + @Test + public void testGetHttpTimeout() { + Request request = Request.builder().build(); + + Assert.assertNull(request.getHttpTimeout()); + } + @Test public void testSetHttpTimeout() { Request request = Request.builder().setHttpTimeout(3000).build(); - Assert.assertEquals(3000, request.getHttpTimeout()); + Assert.assertEquals(Integer.valueOf(3000), request.getHttpTimeout()); } } diff --git a/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java b/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java index fe635a2b75..a254efe382 100644 --- a/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java +++ b/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java @@ -42,6 +42,7 @@ import org.apache.http.NoHttpResponseException; import org.apache.http.conn.HttpHostConnectException; import org.hamcrest.CoreMatchers; +import org.junit.After; import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -115,6 +116,11 @@ public void setUp() throws IOException { Mockito.when(mockHttpResponse.getHeaders()).thenReturn(new HttpHeaders()); } + @After + public void tearDown() { + System.clearProperty("jib.httpTimeout"); + } + @Test public void testCall_httpsPeerUnverified() throws IOException, RegistryException { verifyRetriesWithHttp(SSLPeerUnverifiedException.class); @@ -256,7 +262,7 @@ public void testCall_disallowInsecure() throws IOException, RegistryException { } @Test - public void testHttpTimeout_default20000() throws IOException, RegistryException { + public void testHttpTimeout_propertyNotSet() throws IOException, RegistryException { MockConnection mockConnection = new MockConnection((httpMethod, request) -> mockResponse); Mockito.when(mockConnectionFactory.apply(Mockito.any())).thenReturn(mockConnection); Mockito.when(mockResponse.getBody()).thenReturn(Mockito.mock(Blob.class)); @@ -264,7 +270,7 @@ public void testHttpTimeout_default20000() throws IOException, RegistryException Assert.assertNull(System.getProperty("jib.httpTimeout")); testRegistryEndpointCallerSecure.call(); - Assert.assertEquals(20000, mockConnection.getRequestedHttpTimeout()); + Assert.assertNull(mockConnection.getRequestedHttpTimeout()); } @Test @@ -276,7 +282,7 @@ public void testHttpTimeout_stringValue() throws IOException, RegistryException System.setProperty("jib.httpTimeout", "random string"); testRegistryEndpointCallerSecure.call(); - Assert.assertEquals(20000, mockConnection.getRequestedHttpTimeout()); + Assert.assertNull(mockConnection.getRequestedHttpTimeout()); } @Test @@ -288,7 +294,7 @@ public void testHttpTimeout_negativeValue() throws IOException, RegistryExceptio System.setProperty("jib.httpTimeout", "-1"); testRegistryEndpointCallerSecure.call(); - Assert.assertEquals(20000, mockConnection.getRequestedHttpTimeout()); + Assert.assertNull(mockConnection.getRequestedHttpTimeout()); } @Test @@ -301,7 +307,20 @@ public void testHttpTimeout_0accepted() throws IOException, RegistryException { Mockito.when(mockResponse.getBody()).thenReturn(Mockito.mock(Blob.class)); testRegistryEndpointCallerSecure.call(); - Assert.assertEquals(0, mockConnection.getRequestedHttpTimeout()); + Assert.assertEquals(Integer.valueOf(0), mockConnection.getRequestedHttpTimeout()); + } + + @Test + public void testHttpTimeout() throws IOException, RegistryException { + System.setProperty("jib.httpTimeout", "7593"); + + MockConnection mockConnection = new MockConnection((httpMethod, request) -> mockResponse); + Mockito.when(mockConnectionFactory.apply(Mockito.any())).thenReturn(mockConnection); + + Mockito.when(mockResponse.getBody()).thenReturn(Mockito.mock(Blob.class)); + testRegistryEndpointCallerSecure.call(); + + Assert.assertEquals(Integer.valueOf(7593), mockConnection.getRequestedHttpTimeout()); } /** Verifies a request is retried with HTTP protocol if {@code exceptionClass} is thrown. */ From b561e61f0c03bcbaa91d86f81d46be616f6769d5 Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Wed, 18 Jul 2018 18:21:00 -0400 Subject: [PATCH 04/15] Warn invalid jib.httpTimeout values --- .../tools/jib/maven/BuildDockerMojo.java | 1 + .../cloud/tools/jib/maven/BuildImageMojo.java | 1 + .../cloud/tools/jib/maven/BuildTarMojo.java | 1 + .../tools/jib/maven/DockerContextMojo.java | 1 + .../jib/maven/JibPluginConfiguration.java | 11 +++++++ .../jib/maven/JibPluginConfigurationTest.java | 29 +++++++++++++++++++ 6 files changed, 44 insertions(+) diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java index e41d7dca77..258322bb64 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java @@ -58,6 +58,7 @@ public class BuildDockerMojo extends JibPluginConfiguration { public void execute() throws MojoExecutionException { MavenBuildLogger mavenBuildLogger = new MavenBuildLogger(getLog()); handleDeprecatedParameters(mavenBuildLogger); + checkHttpTimeoutSystemProperty(mavenBuildLogger); if (!new DockerClient().isDockerInstalled()) { throw new MojoExecutionException(HELPFUL_SUGGESTIONS.forDockerNotInstalled()); diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java index 5c028bc191..d323f1875e 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java @@ -61,6 +61,7 @@ public class BuildImageMojo extends JibPluginConfiguration { public void execute() throws MojoExecutionException, MojoFailureException { MavenBuildLogger mavenBuildLogger = new MavenBuildLogger(getLog()); handleDeprecatedParameters(mavenBuildLogger); + checkHttpTimeoutSystemProperty(mavenBuildLogger); // Validates 'format'. if (Arrays.stream(ImageFormat.values()).noneMatch(value -> value.name().equals(getFormat()))) { diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java index ebe232ce01..15a0e2b3c5 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java @@ -60,6 +60,7 @@ public class BuildTarMojo extends JibPluginConfiguration { public void execute() throws MojoExecutionException { MavenBuildLogger mavenBuildLogger = new MavenBuildLogger(getLog()); handleDeprecatedParameters(mavenBuildLogger); + checkHttpTimeoutSystemProperty(mavenBuildLogger); // Parses 'from' and 'to' into image reference. MavenProjectProperties mavenProjectProperties = diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/DockerContextMojo.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/DockerContextMojo.java index 855923e4f6..ab172e7e50 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/DockerContextMojo.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/DockerContextMojo.java @@ -49,6 +49,7 @@ public class DockerContextMojo extends JibPluginConfiguration { public void execute() throws MojoExecutionException, MojoFailureException { MavenBuildLogger mavenBuildLogger = new MavenBuildLogger(getLog()); handleDeprecatedParameters(mavenBuildLogger); + checkHttpTimeoutSystemProperty(mavenBuildLogger); Preconditions.checkNotNull(targetDir); diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java index 28b16c082c..07451886d2 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java @@ -95,6 +95,17 @@ static ImageReference parseImageReference(String image, String type) { } } + static void checkHttpTimeoutSystemProperty(BuildLogger logger) { + try { + String value = System.getProperty("jib.httpTimeout"); + if (value != null && Integer.parseInt(value) < 0) { + logger.warn("Ignoring negative value of jib.httpTimeout; using the default timeout."); + } + } catch (NumberFormatException ex) { + logger.warn("Ignoring non-integer value of jib.httpTimeout; using the default timeout."); + } + } + /** * Warns about deprecated parameters in use. * diff --git a/jib-maven-plugin/src/test/java/com/google/cloud/tools/jib/maven/JibPluginConfigurationTest.java b/jib-maven-plugin/src/test/java/com/google/cloud/tools/jib/maven/JibPluginConfigurationTest.java index 67b3c9dabf..a17f238821 100644 --- a/jib-maven-plugin/src/test/java/com/google/cloud/tools/jib/maven/JibPluginConfigurationTest.java +++ b/jib-maven-plugin/src/test/java/com/google/cloud/tools/jib/maven/JibPluginConfigurationTest.java @@ -19,6 +19,7 @@ import com.google.cloud.tools.jib.builder.BuildLogger; import java.nio.file.Paths; import java.util.Arrays; +import org.junit.After; import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; @@ -32,6 +33,11 @@ public class JibPluginConfigurationTest { @Mock private BuildLogger mockLogger; + @After + public void tearDown() { + System.clearProperty("jib.httpTimeout"); + } + @Test public void testHandleDeprecatedParameters() { JibPluginConfiguration testPluginConfiguration = @@ -66,4 +72,27 @@ public void execute() {} Assert.assertEquals("OCI", testPluginConfiguration.getFormat()); Assert.assertEquals(Paths.get("some/path"), testPluginConfiguration.getExtraDirectory()); } + + @Test + public void testCheckHttpTimeoutSystemProperty() { + Assert.assertNull(System.getProperty("jib.httpTimeout")); + JibPluginConfiguration.checkHttpTimeoutSystemProperty(mockLogger); + Mockito.verify(mockLogger, Mockito.never()).warn(Mockito.any()); + } + + @Test + public void testCheckHttpTimeoutSystemProperty_stringValue() { + System.setProperty("jib.httpTimeout", "random string"); + JibPluginConfiguration.checkHttpTimeoutSystemProperty(mockLogger); + Mockito.verify(mockLogger) + .warn("Ignoring non-integer value of jib.httpTimeout; using the default timeout."); + } + + @Test + public void testCheckHttpTimeoutSystemProperty_negativeValue() { + System.setProperty("jib.httpTimeout", "-80"); + JibPluginConfiguration.checkHttpTimeoutSystemProperty(mockLogger); + Mockito.verify(mockLogger) + .warn("Ignoring negative value of jib.httpTimeout; using the default timeout."); + } } From d4251d009d5b4b638f3be3eb733c78b9c145831a Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Wed, 18 Jul 2018 18:34:57 -0400 Subject: [PATCH 05/15] Warn invalid jib.httpTimeout values - gradle side --- jib-gradle-plugin/bin/.gitignore | 4 +++ .../tools/jib/gradle/BuildDockerTask.java | 1 + .../tools/jib/gradle/BuildImageTask.java | 1 + .../cloud/tools/jib/gradle/BuildTarTask.java | 1 + .../tools/jib/gradle/DockerContextTask.java | 1 + .../cloud/tools/jib/gradle/JibExtension.java | 11 +++++++ .../tools/jib/gradle/JibExtensionTest.java | 29 +++++++++++++++++++ 7 files changed, 48 insertions(+) create mode 100644 jib-gradle-plugin/bin/.gitignore diff --git a/jib-gradle-plugin/bin/.gitignore b/jib-gradle-plugin/bin/.gitignore new file mode 100644 index 0000000000..8c0d75fc1a --- /dev/null +++ b/jib-gradle-plugin/bin/.gitignore @@ -0,0 +1,4 @@ +/integrationTest/ +/main/ +/test/ +/default/ diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java index 7415aed902..12eb60cc67 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java @@ -87,6 +87,7 @@ public void buildDocker() throws InvalidImageReferenceException, IOException { Preconditions.checkNotNull(jibExtension); GradleBuildLogger gradleBuildLogger = new GradleBuildLogger(getLogger()); jibExtension.handleDeprecatedParameters(gradleBuildLogger); + JibExtension.checkHttpTimeoutSystemProperty(gradleBuildLogger); if (Boolean.getBoolean("sendCredentialsOverHttp")) { gradleBuildLogger.warn( diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildImageTask.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildImageTask.java index 497f2da9de..0052806a72 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildImageTask.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildImageTask.java @@ -83,6 +83,7 @@ public void buildImage() throws InvalidImageReferenceException, IOException { Preconditions.checkNotNull(jibExtension); GradleBuildLogger gradleBuildLogger = new GradleBuildLogger(getLogger()); jibExtension.handleDeprecatedParameters(gradleBuildLogger); + JibExtension.checkHttpTimeoutSystemProperty(gradleBuildLogger); if (Strings.isNullOrEmpty(jibExtension.getTargetImage())) { throw new GradleException( diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java index 35fab736eb..8b41547874 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java @@ -115,6 +115,7 @@ public void buildTar() throws InvalidImageReferenceException, IOException { Preconditions.checkNotNull(jibExtension); GradleBuildLogger gradleBuildLogger = new GradleBuildLogger(getLogger()); jibExtension.handleDeprecatedParameters(gradleBuildLogger); + JibExtension.checkHttpTimeoutSystemProperty(gradleBuildLogger); if (Boolean.getBoolean("sendCredentialsOverHttp")) { gradleBuildLogger.warn( diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/DockerContextTask.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/DockerContextTask.java index 5d80e7932b..92ed34e9b9 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/DockerContextTask.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/DockerContextTask.java @@ -100,6 +100,7 @@ public void generateDockerContext() { GradleBuildLogger gradleBuildLogger = new GradleBuildLogger(getLogger()); jibExtension.handleDeprecatedParameters(gradleBuildLogger); + JibExtension.checkHttpTimeoutSystemProperty(gradleBuildLogger); GradleProjectProperties gradleProjectProperties = GradleProjectProperties.getForProject(getProject(), gradleBuildLogger); diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java index 75c4b3783b..2587b595fa 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java @@ -72,6 +72,17 @@ private static Path resolveDefaultExtraDirectory(Path projectDirectory) { return projectDirectory.resolve("src").resolve("main").resolve("jib"); } + static void checkHttpTimeoutSystemProperty(BuildLogger logger) { + try { + String value = System.getProperty("jib.httpTimeout"); + if (value != null && Integer.parseInt(value) < 0) { + logger.warn("Ignoring negative value of jib.httpTimeout; using the default timeout."); + } + } catch (NumberFormatException ex) { + logger.warn("Ignoring non-integer value of jib.httpTimeout; using the default timeout."); + } + } + private final ImageConfiguration from; private final ImageConfiguration to; private final ContainerParameters container; diff --git a/jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java b/jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java index 66cf5785c3..561851fbe1 100644 --- a/jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java +++ b/jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java @@ -24,6 +24,7 @@ import java.util.Collections; import org.gradle.api.Project; import org.gradle.testfixtures.ProjectBuilder; +import org.junit.After; import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -49,6 +50,11 @@ public void setUp() { .create(JibPlugin.JIB_EXTENSION_NAME, JibExtension.class, fakeProject); } + @After + public void tearDown() { + System.clearProperty("jib.httpTimeout"); + } + @Test public void testFrom() { Assert.assertEquals("gcr.io/distroless/java", testJibExtension.getFrom().getImage()); @@ -151,4 +157,27 @@ public void testHandleDeprecatedParameters() { Assert.assertEquals(Arrays.asList("arg1", "arg2", "arg3"), testJibExtension.getArgs()); Assert.assertEquals(OCIManifestTemplate.class, testJibExtension.getFormat()); } + + @Test + public void testCheckHttpTimeoutSystemProperty() { + Assert.assertNull(System.getProperty("jib.httpTimeout")); + JibExtension.checkHttpTimeoutSystemProperty(mockLogger); + Mockito.verify(mockLogger, Mockito.never()).warn(Mockito.any()); + } + + @Test + public void testCheckHttpTimeoutSystemProperty_stringValue() { + System.setProperty("jib.httpTimeout", "random string"); + JibExtension.checkHttpTimeoutSystemProperty(mockLogger); + Mockito.verify(mockLogger) + .warn("Ignoring non-integer value of jib.httpTimeout; using the default timeout."); + } + + @Test + public void testCheckHttpTimeoutSystemProperty_negativeValue() { + System.setProperty("jib.httpTimeout", "-80"); + JibExtension.checkHttpTimeoutSystemProperty(mockLogger); + Mockito.verify(mockLogger) + .warn("Ignoring negative value of jib.httpTimeout; using the default timeout."); + } } From 6856f29027574cd7747397c0be9f7542a6ba8a86 Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Wed, 18 Jul 2018 18:37:47 -0400 Subject: [PATCH 06/15] Format code --- .../com/google/cloud/tools/jib/gradle/JibExtensionTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java b/jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java index 561851fbe1..c430dcc09c 100644 --- a/jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java +++ b/jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java @@ -165,7 +165,7 @@ public void testCheckHttpTimeoutSystemProperty() { Mockito.verify(mockLogger, Mockito.never()).warn(Mockito.any()); } - @Test + @Test public void testCheckHttpTimeoutSystemProperty_stringValue() { System.setProperty("jib.httpTimeout", "random string"); JibExtension.checkHttpTimeoutSystemProperty(mockLogger); From 93c7cb869882deef1ef131aa15dd0f68a5c4f27c Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Wed, 18 Jul 2018 18:57:49 -0400 Subject: [PATCH 07/15] Add Javadocs --- jib-core/bin/.gitignore | 3 +++ .../java/com/google/cloud/tools/jib/gradle/JibExtension.java | 5 +++++ .../google/cloud/tools/jib/maven/JibPluginConfiguration.java | 5 +++++ 3 files changed, 13 insertions(+) create mode 100644 jib-core/bin/.gitignore diff --git a/jib-core/bin/.gitignore b/jib-core/bin/.gitignore new file mode 100644 index 0000000000..0dea1861bf --- /dev/null +++ b/jib-core/bin/.gitignore @@ -0,0 +1,3 @@ +/integrationTest/ +/main/ +/test/ diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java index 2587b595fa..5de75d80d4 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java @@ -72,6 +72,11 @@ private static Path resolveDefaultExtraDirectory(Path projectDirectory) { return projectDirectory.resolve("src").resolve("main").resolve("jib"); } + /** + * Warns about invalid values for the {@code jib.httpTimeout} system property. + * + * @param logger The logger used to print the warnings + */ static void checkHttpTimeoutSystemProperty(BuildLogger logger) { try { String value = System.getProperty("jib.httpTimeout"); diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java index 07451886d2..aeaf22f82f 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java @@ -95,6 +95,11 @@ static ImageReference parseImageReference(String image, String type) { } } + /** + * Warns about invalid values for the {@code jib.httpTimeout} system property. + * + * @param logger The logger used to print the warnings + */ static void checkHttpTimeoutSystemProperty(BuildLogger logger) { try { String value = System.getProperty("jib.httpTimeout"); From 5643bf3bde0dc89e0255d54fe7854024faa00bcb Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Thu, 19 Jul 2018 10:32:27 -0400 Subject: [PATCH 08/15] Opt for simple code --- .../com/google/cloud/tools/jib/http/Request.java | 3 ++- .../tools/jib/registry/RegistryAuthenticator.java | 12 ++---------- .../tools/jib/registry/RegistryEndpointCaller.java | 11 +---------- .../jib/registry/RegistryEndpointCallerTest.java | 6 +++++- 4 files changed, 10 insertions(+), 22 deletions(-) diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/http/Request.java b/jib-core/src/main/java/com/google/cloud/tools/jib/http/Request.java index 8a94d5a1f1..6c00d8ace3 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/http/Request.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/http/Request.java @@ -78,7 +78,8 @@ public Builder setUserAgent(String userAgent) { } /** - * Sets the HTTP connection and read timeout in milliseconds. + * Sets the HTTP connection and read timeout in milliseconds. {@code null} uses the default + * timeout and {@code 0} an infinite timeout. * * @param httpTimeout timeout in milliseconds * @return this diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAuthenticator.java b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAuthenticator.java index 0e4c2f941f..eb1c5072c2 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAuthenticator.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAuthenticator.java @@ -103,15 +103,6 @@ public static Initializer initializer(String serverUrl, String repository) { return new Initializer(serverUrl, repository); } - @Nullable - private static Integer getHttpTimeout() { - Integer httpTimeout = Integer.getInteger("jib.httpTimeout"); - if (httpTimeout != null && httpTimeout >= 0) { - return httpTimeout; - } - return null; - } - // TODO: Replace with a WWW-Authenticate header parser. /** * Instantiates from parsing a {@code WWW-Authenticate} header. @@ -248,7 +239,8 @@ private Authorization authenticate(String scope) throws RegistryAuthenticationFa URL authenticationUrl = getAuthenticationUrl(scope); try (Connection connection = new Connection(authenticationUrl)) { - Request.Builder requestBuilder = Request.builder().setHttpTimeout(getHttpTimeout()); + Request.Builder requestBuilder = + Request.builder().setHttpTimeout(Integer.getInteger("jib.httpTimeout")); if (authorization != null) { requestBuilder.setAuthorization(authorization); } diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java index a43aa354df..2848c664bf 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java @@ -69,15 +69,6 @@ static class RequestState { } } - @Nullable - private static Integer getHttpTimeout() { - Integer httpTimeout = Integer.getInteger("jib.httpTimeout"); - if (httpTimeout != null && httpTimeout >= 0) { - return httpTimeout; - } - return null; - } - /** Makes a {@link Connection} to the specified {@link URL}. */ private final Function connectionFactory; @@ -171,7 +162,7 @@ T call(RequestState requestState) throws IOException, RegistryException { Request.Builder requestBuilder = Request.builder() .setUserAgent(userAgent) - .setHttpTimeout(getHttpTimeout()) + .setHttpTimeout(Integer.getInteger("jib.httpTimeout")) .setAccept(registryEndpointProvider.getAccept()) .setBody(registryEndpointProvider.getContent()); // Only sends authorization if using HTTPS. diff --git a/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java b/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java index a254efe382..9a0b8747dd 100644 --- a/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java +++ b/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java @@ -270,6 +270,8 @@ public void testHttpTimeout_propertyNotSet() throws IOException, RegistryExcepti Assert.assertNull(System.getProperty("jib.httpTimeout")); testRegistryEndpointCallerSecure.call(); + // We fall back to the default timeout: + // https://github.com/GoogleContainerTools/jib/pull/656#discussion_r203562639 Assert.assertNull(mockConnection.getRequestedHttpTimeout()); } @@ -294,7 +296,9 @@ public void testHttpTimeout_negativeValue() throws IOException, RegistryExceptio System.setProperty("jib.httpTimeout", "-1"); testRegistryEndpointCallerSecure.call(); - Assert.assertNull(mockConnection.getRequestedHttpTimeout()); + // We let the negative value pass through: + // https://github.com/GoogleContainerTools/jib/pull/656#discussion_r203562639 + Assert.assertEquals(Integer.valueOf(-1), mockConnection.getRequestedHttpTimeout()); } @Test From 95eb9bded4953aa35f4c63bc46adc6ad5913196f Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Thu, 19 Jul 2018 10:50:19 -0400 Subject: [PATCH 09/15] Error instead of warn --- .../tools/jib/maven/BuildDockerMojo.java | 2 +- .../cloud/tools/jib/maven/BuildImageMojo.java | 2 +- .../cloud/tools/jib/maven/BuildTarMojo.java | 2 +- .../tools/jib/maven/DockerContextMojo.java | 2 +- .../jib/maven/JibPluginConfiguration.java | 6 ++--- .../jib/maven/JibPluginConfigurationTest.java | 23 +++++++++++-------- 6 files changed, 21 insertions(+), 16 deletions(-) diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java index 258322bb64..5a770ccf98 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java @@ -58,7 +58,7 @@ public class BuildDockerMojo extends JibPluginConfiguration { public void execute() throws MojoExecutionException { MavenBuildLogger mavenBuildLogger = new MavenBuildLogger(getLog()); handleDeprecatedParameters(mavenBuildLogger); - checkHttpTimeoutSystemProperty(mavenBuildLogger); + checkHttpTimeoutSystemProperty(); if (!new DockerClient().isDockerInstalled()) { throw new MojoExecutionException(HELPFUL_SUGGESTIONS.forDockerNotInstalled()); diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java index d323f1875e..1b4d165c64 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java @@ -61,7 +61,7 @@ public class BuildImageMojo extends JibPluginConfiguration { public void execute() throws MojoExecutionException, MojoFailureException { MavenBuildLogger mavenBuildLogger = new MavenBuildLogger(getLog()); handleDeprecatedParameters(mavenBuildLogger); - checkHttpTimeoutSystemProperty(mavenBuildLogger); + checkHttpTimeoutSystemProperty(); // Validates 'format'. if (Arrays.stream(ImageFormat.values()).noneMatch(value -> value.name().equals(getFormat()))) { diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java index 15a0e2b3c5..3d94b59f2c 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java @@ -60,7 +60,7 @@ public class BuildTarMojo extends JibPluginConfiguration { public void execute() throws MojoExecutionException { MavenBuildLogger mavenBuildLogger = new MavenBuildLogger(getLog()); handleDeprecatedParameters(mavenBuildLogger); - checkHttpTimeoutSystemProperty(mavenBuildLogger); + checkHttpTimeoutSystemProperty(); // Parses 'from' and 'to' into image reference. MavenProjectProperties mavenProjectProperties = diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/DockerContextMojo.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/DockerContextMojo.java index ab172e7e50..f696625f64 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/DockerContextMojo.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/DockerContextMojo.java @@ -49,7 +49,7 @@ public class DockerContextMojo extends JibPluginConfiguration { public void execute() throws MojoExecutionException, MojoFailureException { MavenBuildLogger mavenBuildLogger = new MavenBuildLogger(getLog()); handleDeprecatedParameters(mavenBuildLogger); - checkHttpTimeoutSystemProperty(mavenBuildLogger); + checkHttpTimeoutSystemProperty(); Preconditions.checkNotNull(targetDir); diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java index aeaf22f82f..4f4018d413 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java @@ -100,14 +100,14 @@ static ImageReference parseImageReference(String image, String type) { * * @param logger The logger used to print the warnings */ - static void checkHttpTimeoutSystemProperty(BuildLogger logger) { + static void checkHttpTimeoutSystemProperty() { try { String value = System.getProperty("jib.httpTimeout"); if (value != null && Integer.parseInt(value) < 0) { - logger.warn("Ignoring negative value of jib.httpTimeout; using the default timeout."); + throw new IllegalArgumentException("negative value of jib.httpTimeout"); } } catch (NumberFormatException ex) { - logger.warn("Ignoring non-integer value of jib.httpTimeout; using the default timeout."); + throw new IllegalArgumentException("non-integer value of jib.httpTimeout"); } } diff --git a/jib-maven-plugin/src/test/java/com/google/cloud/tools/jib/maven/JibPluginConfigurationTest.java b/jib-maven-plugin/src/test/java/com/google/cloud/tools/jib/maven/JibPluginConfigurationTest.java index a17f238821..9a192842f5 100644 --- a/jib-maven-plugin/src/test/java/com/google/cloud/tools/jib/maven/JibPluginConfigurationTest.java +++ b/jib-maven-plugin/src/test/java/com/google/cloud/tools/jib/maven/JibPluginConfigurationTest.java @@ -74,25 +74,30 @@ public void execute() {} } @Test - public void testCheckHttpTimeoutSystemProperty() { + public void testCheckHttpTimeoutSystemProperty_ok() { Assert.assertNull(System.getProperty("jib.httpTimeout")); - JibPluginConfiguration.checkHttpTimeoutSystemProperty(mockLogger); - Mockito.verify(mockLogger, Mockito.never()).warn(Mockito.any()); + JibPluginConfiguration.checkHttpTimeoutSystemProperty(); } @Test public void testCheckHttpTimeoutSystemProperty_stringValue() { System.setProperty("jib.httpTimeout", "random string"); - JibPluginConfiguration.checkHttpTimeoutSystemProperty(mockLogger); - Mockito.verify(mockLogger) - .warn("Ignoring non-integer value of jib.httpTimeout; using the default timeout."); + try { + JibPluginConfiguration.checkHttpTimeoutSystemProperty(); + Assert.fail("Should error with a non-integer timeout"); + } catch (IllegalArgumentException ex) { + Assert.assertEquals("non-integer value of jib.httpTimeout", ex.getMessage()); + } } @Test public void testCheckHttpTimeoutSystemProperty_negativeValue() { System.setProperty("jib.httpTimeout", "-80"); - JibPluginConfiguration.checkHttpTimeoutSystemProperty(mockLogger); - Mockito.verify(mockLogger) - .warn("Ignoring negative value of jib.httpTimeout; using the default timeout."); + try { + JibPluginConfiguration.checkHttpTimeoutSystemProperty(); + Assert.fail("Should error with a negative timeout"); + } catch (IllegalArgumentException ex) { + Assert.assertEquals("negative value of jib.httpTimeout", ex.getMessage()); + } } } From 28a96ea46a392cdc7274764e8d83a17e37555878 Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Thu, 19 Jul 2018 10:53:58 -0400 Subject: [PATCH 10/15] Error on invalid timeouts - gradle plugin --- .../tools/jib/gradle/BuildDockerTask.java | 2 +- .../tools/jib/gradle/BuildImageTask.java | 2 +- .../cloud/tools/jib/gradle/BuildTarTask.java | 2 +- .../tools/jib/gradle/DockerContextTask.java | 2 +- .../cloud/tools/jib/gradle/JibExtension.java | 12 ++++------ .../tools/jib/gradle/JibExtensionTest.java | 23 +++++++++++-------- 6 files changed, 22 insertions(+), 21 deletions(-) diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java index 12eb60cc67..2c13d83db6 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java @@ -87,7 +87,7 @@ public void buildDocker() throws InvalidImageReferenceException, IOException { Preconditions.checkNotNull(jibExtension); GradleBuildLogger gradleBuildLogger = new GradleBuildLogger(getLogger()); jibExtension.handleDeprecatedParameters(gradleBuildLogger); - JibExtension.checkHttpTimeoutSystemProperty(gradleBuildLogger); + JibExtension.checkHttpTimeoutSystemProperty(); if (Boolean.getBoolean("sendCredentialsOverHttp")) { gradleBuildLogger.warn( diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildImageTask.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildImageTask.java index 0052806a72..1287c5ae10 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildImageTask.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildImageTask.java @@ -83,7 +83,7 @@ public void buildImage() throws InvalidImageReferenceException, IOException { Preconditions.checkNotNull(jibExtension); GradleBuildLogger gradleBuildLogger = new GradleBuildLogger(getLogger()); jibExtension.handleDeprecatedParameters(gradleBuildLogger); - JibExtension.checkHttpTimeoutSystemProperty(gradleBuildLogger); + JibExtension.checkHttpTimeoutSystemProperty(); if (Strings.isNullOrEmpty(jibExtension.getTargetImage())) { throw new GradleException( diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java index 8b41547874..b0eabcf132 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java @@ -115,7 +115,7 @@ public void buildTar() throws InvalidImageReferenceException, IOException { Preconditions.checkNotNull(jibExtension); GradleBuildLogger gradleBuildLogger = new GradleBuildLogger(getLogger()); jibExtension.handleDeprecatedParameters(gradleBuildLogger); - JibExtension.checkHttpTimeoutSystemProperty(gradleBuildLogger); + JibExtension.checkHttpTimeoutSystemProperty(); if (Boolean.getBoolean("sendCredentialsOverHttp")) { gradleBuildLogger.warn( diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/DockerContextTask.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/DockerContextTask.java index 92ed34e9b9..20ee64580f 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/DockerContextTask.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/DockerContextTask.java @@ -100,7 +100,7 @@ public void generateDockerContext() { GradleBuildLogger gradleBuildLogger = new GradleBuildLogger(getLogger()); jibExtension.handleDeprecatedParameters(gradleBuildLogger); - JibExtension.checkHttpTimeoutSystemProperty(gradleBuildLogger); + JibExtension.checkHttpTimeoutSystemProperty(); GradleProjectProperties gradleProjectProperties = GradleProjectProperties.getForProject(getProject(), gradleBuildLogger); diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java index 5de75d80d4..9a63efeb42 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java @@ -72,19 +72,15 @@ private static Path resolveDefaultExtraDirectory(Path projectDirectory) { return projectDirectory.resolve("src").resolve("main").resolve("jib"); } - /** - * Warns about invalid values for the {@code jib.httpTimeout} system property. - * - * @param logger The logger used to print the warnings - */ - static void checkHttpTimeoutSystemProperty(BuildLogger logger) { + /** Errors on invalid values for the {@code jib.httpTimeout} system property. */ + static void checkHttpTimeoutSystemProperty() { try { String value = System.getProperty("jib.httpTimeout"); if (value != null && Integer.parseInt(value) < 0) { - logger.warn("Ignoring negative value of jib.httpTimeout; using the default timeout."); + throw new IllegalArgumentException("negative value of jib.httpTimeout"); } } catch (NumberFormatException ex) { - logger.warn("Ignoring non-integer value of jib.httpTimeout; using the default timeout."); + throw new IllegalArgumentException("non-integer value of jib.httpTimeout"); } } diff --git a/jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java b/jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java index c430dcc09c..58a2959a0d 100644 --- a/jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java +++ b/jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java @@ -159,25 +159,30 @@ public void testHandleDeprecatedParameters() { } @Test - public void testCheckHttpTimeoutSystemProperty() { + public void testCheckHttpTimeoutSystemProperty_ok() { Assert.assertNull(System.getProperty("jib.httpTimeout")); - JibExtension.checkHttpTimeoutSystemProperty(mockLogger); - Mockito.verify(mockLogger, Mockito.never()).warn(Mockito.any()); + JibExtension.checkHttpTimeoutSystemProperty(); } @Test public void testCheckHttpTimeoutSystemProperty_stringValue() { System.setProperty("jib.httpTimeout", "random string"); - JibExtension.checkHttpTimeoutSystemProperty(mockLogger); - Mockito.verify(mockLogger) - .warn("Ignoring non-integer value of jib.httpTimeout; using the default timeout."); + try { + JibExtension.checkHttpTimeoutSystemProperty(); + Assert.fail("Should error with a non-integer timeout"); + } catch (IllegalArgumentException ex) { + Assert.assertEquals("non-integer value of jib.httpTimeout", ex.getMessage()); + } } @Test public void testCheckHttpTimeoutSystemProperty_negativeValue() { System.setProperty("jib.httpTimeout", "-80"); - JibExtension.checkHttpTimeoutSystemProperty(mockLogger); - Mockito.verify(mockLogger) - .warn("Ignoring negative value of jib.httpTimeout; using the default timeout."); + try { + JibExtension.checkHttpTimeoutSystemProperty(); + Assert.fail("Should error with a negative timeout"); + } catch (IllegalArgumentException ex) { + Assert.assertEquals("negative value of jib.httpTimeout", ex.getMessage()); + } } } From 609ac5e1a4cac44b624c546a70388bfa3e24b051 Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Thu, 19 Jul 2018 14:31:22 -0400 Subject: [PATCH 11/15] Fix Javadoc --- .../cloud/tools/jib/maven/JibPluginConfiguration.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java index 4f4018d413..0d0b8f695f 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java @@ -95,11 +95,7 @@ static ImageReference parseImageReference(String image, String type) { } } - /** - * Warns about invalid values for the {@code jib.httpTimeout} system property. - * - * @param logger The logger used to print the warnings - */ + /** Errors about invalid values for the {@code jib.httpTimeout} system property. */ static void checkHttpTimeoutSystemProperty() { try { String value = System.getProperty("jib.httpTimeout"); From 4c98316712efc575b066e1d27e7367beeee8ff01 Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Thu, 19 Jul 2018 14:35:37 -0400 Subject: [PATCH 12/15] Update CHANGELOG files --- jib-gradle-plugin/CHANGELOG.md | 1 + jib-maven-plugin/CHANGELOG.md | 1 + 2 files changed, 2 insertions(+) diff --git a/jib-gradle-plugin/CHANGELOG.md b/jib-gradle-plugin/CHANGELOG.md index eca7d25e39..1fc7c0dcbd 100644 --- a/jib-gradle-plugin/CHANGELOG.md +++ b/jib-gradle-plugin/CHANGELOG.md @@ -9,6 +9,7 @@ All notable changes to this project will be documented in this file. - `jibBuildTar` task to build an image tarball at `build/jib-image.tar`, which can be loaded into docker using `docker load` ([#514](https://github.com/GoogleContainerTools/jib/issues/514)) - `container.useCurrentTimestamp` parameter to set the image creation time to the build time ([#413](https://github.com/GoogleContainerTools/jib/issues/413)) - Authentication over HTTP using the `sendCredentialsOverHttp` system property ([#599](https://github.com/GoogleContainerTools/jib/issues/599)) +- HTTP connection and read timeouts for registry interactions configurable with the `jib.httpTimeout` system property ([#656](https://github.com/GoogleContainerTools/jib/pull/656)) ### Changed diff --git a/jib-maven-plugin/CHANGELOG.md b/jib-maven-plugin/CHANGELOG.md index be80d3ebb0..9e2c20df46 100644 --- a/jib-maven-plugin/CHANGELOG.md +++ b/jib-maven-plugin/CHANGELOG.md @@ -9,6 +9,7 @@ All notable changes to this project will be documented in this file. - `jib:buildTar` goal to build an image tarball at `target/jib-image.tar`, which can be loaded into docker using `docker load` ([#514](https://github.com/GoogleContainerTools/jib/issues/514)) - `` parameter to set the image creation time to the build time ([#413](https://github.com/GoogleContainerTools/jib/issues/413)) - Authentication over HTTP using the `sendCredentialsOverHttp` system property ([#599](https://github.com/GoogleContainerTools/jib/issues/599)) +- HTTP connection and read timeouts for registry interactions configurable with the `jib.httpTimeout` system property ([#656](https://github.com/GoogleContainerTools/jib/pull/656)) ### Changed From ffecc8871d9d3ab56f62fe7f69b1bd21cfc3dfe5 Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Thu, 19 Jul 2018 14:41:20 -0400 Subject: [PATCH 13/15] Delete .gitignore files --- jib-core/bin/.gitignore | 3 --- jib-gradle-plugin/bin/.gitignore | 4 ---- 2 files changed, 7 deletions(-) delete mode 100644 jib-core/bin/.gitignore delete mode 100644 jib-gradle-plugin/bin/.gitignore diff --git a/jib-core/bin/.gitignore b/jib-core/bin/.gitignore deleted file mode 100644 index 0dea1861bf..0000000000 --- a/jib-core/bin/.gitignore +++ /dev/null @@ -1,3 +0,0 @@ -/integrationTest/ -/main/ -/test/ diff --git a/jib-gradle-plugin/bin/.gitignore b/jib-gradle-plugin/bin/.gitignore deleted file mode 100644 index 8c0d75fc1a..0000000000 --- a/jib-gradle-plugin/bin/.gitignore +++ /dev/null @@ -1,4 +0,0 @@ -/integrationTest/ -/main/ -/test/ -/default/ From 539d3cbd261856f085cfa685601ad6e4a265acad Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Thu, 19 Jul 2018 15:29:03 -0400 Subject: [PATCH 14/15] Refactor code and throw Maven/Gradle exception --- .../jib/frontend/SystemPropertyValidator.java | 43 ++++++++++++++ .../frontend/SystemPropertyValidatorTest.java | 58 +++++++++++++++++++ .../tools/jib/gradle/BuildDockerTask.java | 3 +- .../tools/jib/gradle/BuildImageTask.java | 3 +- .../cloud/tools/jib/gradle/BuildTarTask.java | 3 +- .../tools/jib/gradle/DockerContextTask.java | 3 +- .../cloud/tools/jib/gradle/JibExtension.java | 12 ---- .../tools/jib/gradle/JibExtensionTest.java | 34 ----------- .../tools/jib/maven/BuildDockerMojo.java | 3 +- .../cloud/tools/jib/maven/BuildImageMojo.java | 3 +- .../cloud/tools/jib/maven/BuildTarMojo.java | 3 +- .../tools/jib/maven/DockerContextMojo.java | 3 +- .../jib/maven/JibPluginConfiguration.java | 12 ---- .../jib/maven/JibPluginConfigurationTest.java | 34 ----------- 14 files changed, 117 insertions(+), 100 deletions(-) create mode 100644 jib-core/src/main/java/com/google/cloud/tools/jib/frontend/SystemPropertyValidator.java create mode 100644 jib-core/src/test/java/com/google/cloud/tools/jib/frontend/SystemPropertyValidatorTest.java diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/frontend/SystemPropertyValidator.java b/jib-core/src/main/java/com/google/cloud/tools/jib/frontend/SystemPropertyValidator.java new file mode 100644 index 0000000000..2cd6d633ae --- /dev/null +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/frontend/SystemPropertyValidator.java @@ -0,0 +1,43 @@ +/* + * Copyright 2018 Google LLC. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package com.google.cloud.tools.jib.frontend; + +import java.util.function.Function; + +/** Validator for system properties. */ +public class SystemPropertyValidator { + + /** + * Checks the {@code jib.httpTimeout} system property for invalid (non-integer or negative) + * values. + * + * @param exceptionFactory factory to create an exception with the given description + * @param the exception type to throw if invalid values + * @throws T if invalid values + */ + public static void checkHttpTimeoutProperty( + Function exceptionFactory) throws T { + try { + String value = System.getProperty("jib.httpTimeout"); + if (value != null && Integer.parseInt(value) < 0) { + throw exceptionFactory.apply("negative value of jib.httpTimeout"); + } + } catch (NumberFormatException ex) { + throw exceptionFactory.apply("non-integer value of jib.httpTimeout"); + } + } +} diff --git a/jib-core/src/test/java/com/google/cloud/tools/jib/frontend/SystemPropertyValidatorTest.java b/jib-core/src/test/java/com/google/cloud/tools/jib/frontend/SystemPropertyValidatorTest.java new file mode 100644 index 0000000000..9b68702236 --- /dev/null +++ b/jib-core/src/test/java/com/google/cloud/tools/jib/frontend/SystemPropertyValidatorTest.java @@ -0,0 +1,58 @@ +/* + * Copyright 2018 Google LLC. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package com.google.cloud.tools.jib.frontend; + +import org.junit.After; +import org.junit.Assert; +import org.junit.Test; + +/** Tests for {@link SystemPropertyValidator}. */ +public class SystemPropertyValidatorTest { + + @After + public void tearDown() { + System.clearProperty("jib.httpTimeout"); + } + + @Test + public void testCheckHttpTimeoutSystemProperty_ok() throws Exception { + Assert.assertNull(System.getProperty("jib.httpTimeout")); + SystemPropertyValidator.checkHttpTimeoutProperty(Exception::new); + } + + @Test + public void testCheckHttpTimeoutSystemProperty_stringValue() { + System.setProperty("jib.httpTimeout", "random string"); + try { + SystemPropertyValidator.checkHttpTimeoutProperty(Exception::new); + Assert.fail("Should error with a non-integer timeout"); + } catch (Exception ex) { + Assert.assertEquals("non-integer value of jib.httpTimeout", ex.getMessage()); + } + } + + @Test + public void testCheckHttpTimeoutSystemProperty_negativeValue() { + System.setProperty("jib.httpTimeout", "-80"); + try { + SystemPropertyValidator.checkHttpTimeoutProperty(Exception::new); + Assert.fail("Should error with a negative timeout"); + } catch (Exception ex) { + Assert.assertEquals("negative value of jib.httpTimeout", ex.getMessage()); + } + } +} diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java index 2c13d83db6..0b6241d29a 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java @@ -25,6 +25,7 @@ import com.google.cloud.tools.jib.frontend.BuildStepsRunner; import com.google.cloud.tools.jib.frontend.ExposedPortsParser; import com.google.cloud.tools.jib.frontend.HelpfulSuggestions; +import com.google.cloud.tools.jib.frontend.SystemPropertyValidator; import com.google.cloud.tools.jib.http.Authorization; import com.google.cloud.tools.jib.image.ImageReference; import com.google.cloud.tools.jib.image.InvalidImageReferenceException; @@ -87,7 +88,7 @@ public void buildDocker() throws InvalidImageReferenceException, IOException { Preconditions.checkNotNull(jibExtension); GradleBuildLogger gradleBuildLogger = new GradleBuildLogger(getLogger()); jibExtension.handleDeprecatedParameters(gradleBuildLogger); - JibExtension.checkHttpTimeoutSystemProperty(); + SystemPropertyValidator.checkHttpTimeoutProperty(GradleException::new); if (Boolean.getBoolean("sendCredentialsOverHttp")) { gradleBuildLogger.warn( diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildImageTask.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildImageTask.java index 1287c5ae10..4e77100196 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildImageTask.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildImageTask.java @@ -24,6 +24,7 @@ import com.google.cloud.tools.jib.frontend.BuildStepsRunner; import com.google.cloud.tools.jib.frontend.ExposedPortsParser; import com.google.cloud.tools.jib.frontend.HelpfulSuggestions; +import com.google.cloud.tools.jib.frontend.SystemPropertyValidator; import com.google.cloud.tools.jib.http.Authorization; import com.google.cloud.tools.jib.image.ImageReference; import com.google.cloud.tools.jib.image.InvalidImageReferenceException; @@ -83,7 +84,7 @@ public void buildImage() throws InvalidImageReferenceException, IOException { Preconditions.checkNotNull(jibExtension); GradleBuildLogger gradleBuildLogger = new GradleBuildLogger(getLogger()); jibExtension.handleDeprecatedParameters(gradleBuildLogger); - JibExtension.checkHttpTimeoutSystemProperty(); + SystemPropertyValidator.checkHttpTimeoutProperty(GradleException::new); if (Strings.isNullOrEmpty(jibExtension.getTargetImage())) { throw new GradleException( diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java index b0eabcf132..64086da31a 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java @@ -24,6 +24,7 @@ import com.google.cloud.tools.jib.frontend.BuildStepsRunner; import com.google.cloud.tools.jib.frontend.ExposedPortsParser; import com.google.cloud.tools.jib.frontend.HelpfulSuggestions; +import com.google.cloud.tools.jib.frontend.SystemPropertyValidator; import com.google.cloud.tools.jib.http.Authorization; import com.google.cloud.tools.jib.image.ImageReference; import com.google.cloud.tools.jib.image.InvalidImageReferenceException; @@ -115,7 +116,7 @@ public void buildTar() throws InvalidImageReferenceException, IOException { Preconditions.checkNotNull(jibExtension); GradleBuildLogger gradleBuildLogger = new GradleBuildLogger(getLogger()); jibExtension.handleDeprecatedParameters(gradleBuildLogger); - JibExtension.checkHttpTimeoutSystemProperty(); + SystemPropertyValidator.checkHttpTimeoutProperty(GradleException::new); if (Boolean.getBoolean("sendCredentialsOverHttp")) { gradleBuildLogger.warn( diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/DockerContextTask.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/DockerContextTask.java index 20ee64580f..dbe397ba74 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/DockerContextTask.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/DockerContextTask.java @@ -18,6 +18,7 @@ import com.google.cloud.tools.jib.docker.DockerContextGenerator; import com.google.cloud.tools.jib.frontend.ExposedPortsParser; +import com.google.cloud.tools.jib.frontend.SystemPropertyValidator; import com.google.common.base.Preconditions; import com.google.common.io.InsecureRecursiveDeleteException; import java.io.IOException; @@ -100,7 +101,7 @@ public void generateDockerContext() { GradleBuildLogger gradleBuildLogger = new GradleBuildLogger(getLogger()); jibExtension.handleDeprecatedParameters(gradleBuildLogger); - JibExtension.checkHttpTimeoutSystemProperty(); + SystemPropertyValidator.checkHttpTimeoutProperty(GradleException::new); GradleProjectProperties gradleProjectProperties = GradleProjectProperties.getForProject(getProject(), gradleBuildLogger); diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java index 9a63efeb42..75c4b3783b 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java @@ -72,18 +72,6 @@ private static Path resolveDefaultExtraDirectory(Path projectDirectory) { return projectDirectory.resolve("src").resolve("main").resolve("jib"); } - /** Errors on invalid values for the {@code jib.httpTimeout} system property. */ - static void checkHttpTimeoutSystemProperty() { - try { - String value = System.getProperty("jib.httpTimeout"); - if (value != null && Integer.parseInt(value) < 0) { - throw new IllegalArgumentException("negative value of jib.httpTimeout"); - } - } catch (NumberFormatException ex) { - throw new IllegalArgumentException("non-integer value of jib.httpTimeout"); - } - } - private final ImageConfiguration from; private final ImageConfiguration to; private final ContainerParameters container; diff --git a/jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java b/jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java index 58a2959a0d..66cf5785c3 100644 --- a/jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java +++ b/jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java @@ -24,7 +24,6 @@ import java.util.Collections; import org.gradle.api.Project; import org.gradle.testfixtures.ProjectBuilder; -import org.junit.After; import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -50,11 +49,6 @@ public void setUp() { .create(JibPlugin.JIB_EXTENSION_NAME, JibExtension.class, fakeProject); } - @After - public void tearDown() { - System.clearProperty("jib.httpTimeout"); - } - @Test public void testFrom() { Assert.assertEquals("gcr.io/distroless/java", testJibExtension.getFrom().getImage()); @@ -157,32 +151,4 @@ public void testHandleDeprecatedParameters() { Assert.assertEquals(Arrays.asList("arg1", "arg2", "arg3"), testJibExtension.getArgs()); Assert.assertEquals(OCIManifestTemplate.class, testJibExtension.getFormat()); } - - @Test - public void testCheckHttpTimeoutSystemProperty_ok() { - Assert.assertNull(System.getProperty("jib.httpTimeout")); - JibExtension.checkHttpTimeoutSystemProperty(); - } - - @Test - public void testCheckHttpTimeoutSystemProperty_stringValue() { - System.setProperty("jib.httpTimeout", "random string"); - try { - JibExtension.checkHttpTimeoutSystemProperty(); - Assert.fail("Should error with a non-integer timeout"); - } catch (IllegalArgumentException ex) { - Assert.assertEquals("non-integer value of jib.httpTimeout", ex.getMessage()); - } - } - - @Test - public void testCheckHttpTimeoutSystemProperty_negativeValue() { - System.setProperty("jib.httpTimeout", "-80"); - try { - JibExtension.checkHttpTimeoutSystemProperty(); - Assert.fail("Should error with a negative timeout"); - } catch (IllegalArgumentException ex) { - Assert.assertEquals("negative value of jib.httpTimeout", ex.getMessage()); - } - } } diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java index 5a770ccf98..f34f8c9528 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java @@ -25,6 +25,7 @@ import com.google.cloud.tools.jib.frontend.BuildStepsRunner; import com.google.cloud.tools.jib.frontend.ExposedPortsParser; import com.google.cloud.tools.jib.frontend.HelpfulSuggestions; +import com.google.cloud.tools.jib.frontend.SystemPropertyValidator; import com.google.cloud.tools.jib.image.ImageReference; import com.google.cloud.tools.jib.registry.RegistryClient; import com.google.cloud.tools.jib.registry.credentials.RegistryCredentials; @@ -58,7 +59,7 @@ public class BuildDockerMojo extends JibPluginConfiguration { public void execute() throws MojoExecutionException { MavenBuildLogger mavenBuildLogger = new MavenBuildLogger(getLog()); handleDeprecatedParameters(mavenBuildLogger); - checkHttpTimeoutSystemProperty(); + SystemPropertyValidator.checkHttpTimeoutProperty(MojoExecutionException::new); if (!new DockerClient().isDockerInstalled()) { throw new MojoExecutionException(HELPFUL_SUGGESTIONS.forDockerNotInstalled()); diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java index 1b4d165c64..62a840f291 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java @@ -24,6 +24,7 @@ import com.google.cloud.tools.jib.frontend.BuildStepsRunner; import com.google.cloud.tools.jib.frontend.ExposedPortsParser; import com.google.cloud.tools.jib.frontend.HelpfulSuggestions; +import com.google.cloud.tools.jib.frontend.SystemPropertyValidator; import com.google.cloud.tools.jib.image.ImageFormat; import com.google.cloud.tools.jib.image.ImageReference; import com.google.cloud.tools.jib.registry.RegistryClient; @@ -61,7 +62,7 @@ public class BuildImageMojo extends JibPluginConfiguration { public void execute() throws MojoExecutionException, MojoFailureException { MavenBuildLogger mavenBuildLogger = new MavenBuildLogger(getLog()); handleDeprecatedParameters(mavenBuildLogger); - checkHttpTimeoutSystemProperty(); + SystemPropertyValidator.checkHttpTimeoutProperty(MojoExecutionException::new); // Validates 'format'. if (Arrays.stream(ImageFormat.values()).noneMatch(value -> value.name().equals(getFormat()))) { diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java index 3d94b59f2c..4787261e18 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java @@ -24,6 +24,7 @@ import com.google.cloud.tools.jib.frontend.BuildStepsRunner; import com.google.cloud.tools.jib.frontend.ExposedPortsParser; import com.google.cloud.tools.jib.frontend.HelpfulSuggestions; +import com.google.cloud.tools.jib.frontend.SystemPropertyValidator; import com.google.cloud.tools.jib.image.ImageReference; import com.google.cloud.tools.jib.registry.RegistryClient; import com.google.cloud.tools.jib.registry.credentials.RegistryCredentials; @@ -60,7 +61,7 @@ public class BuildTarMojo extends JibPluginConfiguration { public void execute() throws MojoExecutionException { MavenBuildLogger mavenBuildLogger = new MavenBuildLogger(getLog()); handleDeprecatedParameters(mavenBuildLogger); - checkHttpTimeoutSystemProperty(); + SystemPropertyValidator.checkHttpTimeoutProperty(MojoExecutionException::new); // Parses 'from' and 'to' into image reference. MavenProjectProperties mavenProjectProperties = diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/DockerContextMojo.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/DockerContextMojo.java index f696625f64..cb7beccd66 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/DockerContextMojo.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/DockerContextMojo.java @@ -18,6 +18,7 @@ import com.google.cloud.tools.jib.docker.DockerContextGenerator; import com.google.cloud.tools.jib.frontend.ExposedPortsParser; +import com.google.cloud.tools.jib.frontend.SystemPropertyValidator; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.io.InsecureRecursiveDeleteException; @@ -49,7 +50,7 @@ public class DockerContextMojo extends JibPluginConfiguration { public void execute() throws MojoExecutionException, MojoFailureException { MavenBuildLogger mavenBuildLogger = new MavenBuildLogger(getLog()); handleDeprecatedParameters(mavenBuildLogger); - checkHttpTimeoutSystemProperty(); + SystemPropertyValidator.checkHttpTimeoutProperty(MojoExecutionException::new); Preconditions.checkNotNull(targetDir); diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java index 0d0b8f695f..28b16c082c 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java @@ -95,18 +95,6 @@ static ImageReference parseImageReference(String image, String type) { } } - /** Errors about invalid values for the {@code jib.httpTimeout} system property. */ - static void checkHttpTimeoutSystemProperty() { - try { - String value = System.getProperty("jib.httpTimeout"); - if (value != null && Integer.parseInt(value) < 0) { - throw new IllegalArgumentException("negative value of jib.httpTimeout"); - } - } catch (NumberFormatException ex) { - throw new IllegalArgumentException("non-integer value of jib.httpTimeout"); - } - } - /** * Warns about deprecated parameters in use. * diff --git a/jib-maven-plugin/src/test/java/com/google/cloud/tools/jib/maven/JibPluginConfigurationTest.java b/jib-maven-plugin/src/test/java/com/google/cloud/tools/jib/maven/JibPluginConfigurationTest.java index 9a192842f5..67b3c9dabf 100644 --- a/jib-maven-plugin/src/test/java/com/google/cloud/tools/jib/maven/JibPluginConfigurationTest.java +++ b/jib-maven-plugin/src/test/java/com/google/cloud/tools/jib/maven/JibPluginConfigurationTest.java @@ -19,7 +19,6 @@ import com.google.cloud.tools.jib.builder.BuildLogger; import java.nio.file.Paths; import java.util.Arrays; -import org.junit.After; import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; @@ -33,11 +32,6 @@ public class JibPluginConfigurationTest { @Mock private BuildLogger mockLogger; - @After - public void tearDown() { - System.clearProperty("jib.httpTimeout"); - } - @Test public void testHandleDeprecatedParameters() { JibPluginConfiguration testPluginConfiguration = @@ -72,32 +66,4 @@ public void execute() {} Assert.assertEquals("OCI", testPluginConfiguration.getFormat()); Assert.assertEquals(Paths.get("some/path"), testPluginConfiguration.getExtraDirectory()); } - - @Test - public void testCheckHttpTimeoutSystemProperty_ok() { - Assert.assertNull(System.getProperty("jib.httpTimeout")); - JibPluginConfiguration.checkHttpTimeoutSystemProperty(); - } - - @Test - public void testCheckHttpTimeoutSystemProperty_stringValue() { - System.setProperty("jib.httpTimeout", "random string"); - try { - JibPluginConfiguration.checkHttpTimeoutSystemProperty(); - Assert.fail("Should error with a non-integer timeout"); - } catch (IllegalArgumentException ex) { - Assert.assertEquals("non-integer value of jib.httpTimeout", ex.getMessage()); - } - } - - @Test - public void testCheckHttpTimeoutSystemProperty_negativeValue() { - System.setProperty("jib.httpTimeout", "-80"); - try { - JibPluginConfiguration.checkHttpTimeoutSystemProperty(); - Assert.fail("Should error with a negative timeout"); - } catch (IllegalArgumentException ex) { - Assert.assertEquals("negative value of jib.httpTimeout", ex.getMessage()); - } - } } From e452c037fe8d4783883bcfd7a90822efc77c8d35 Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Thu, 19 Jul 2018 15:55:42 -0400 Subject: [PATCH 15/15] Change error wordings / add private constructor --- .../cloud/tools/jib/frontend/SystemPropertyValidator.java | 8 +++++--- .../tools/jib/frontend/SystemPropertyValidatorTest.java | 4 ++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/frontend/SystemPropertyValidator.java b/jib-core/src/main/java/com/google/cloud/tools/jib/frontend/SystemPropertyValidator.java index 2cd6d633ae..e83cb628cd 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/frontend/SystemPropertyValidator.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/frontend/SystemPropertyValidator.java @@ -31,13 +31,15 @@ public class SystemPropertyValidator { */ public static void checkHttpTimeoutProperty( Function exceptionFactory) throws T { + String value = System.getProperty("jib.httpTimeout"); try { - String value = System.getProperty("jib.httpTimeout"); if (value != null && Integer.parseInt(value) < 0) { - throw exceptionFactory.apply("negative value of jib.httpTimeout"); + throw exceptionFactory.apply("jib.httpTimeout cannot be negative: " + value); } } catch (NumberFormatException ex) { - throw exceptionFactory.apply("non-integer value of jib.httpTimeout"); + throw exceptionFactory.apply("jib.httpTimeout must be an integer: " + value); } } + + private SystemPropertyValidator() {} } diff --git a/jib-core/src/test/java/com/google/cloud/tools/jib/frontend/SystemPropertyValidatorTest.java b/jib-core/src/test/java/com/google/cloud/tools/jib/frontend/SystemPropertyValidatorTest.java index 9b68702236..ef7feb0c30 100644 --- a/jib-core/src/test/java/com/google/cloud/tools/jib/frontend/SystemPropertyValidatorTest.java +++ b/jib-core/src/test/java/com/google/cloud/tools/jib/frontend/SystemPropertyValidatorTest.java @@ -41,7 +41,7 @@ public void testCheckHttpTimeoutSystemProperty_stringValue() { SystemPropertyValidator.checkHttpTimeoutProperty(Exception::new); Assert.fail("Should error with a non-integer timeout"); } catch (Exception ex) { - Assert.assertEquals("non-integer value of jib.httpTimeout", ex.getMessage()); + Assert.assertEquals("jib.httpTimeout must be an integer: random string", ex.getMessage()); } } @@ -52,7 +52,7 @@ public void testCheckHttpTimeoutSystemProperty_negativeValue() { SystemPropertyValidator.checkHttpTimeoutProperty(Exception::new); Assert.fail("Should error with a negative timeout"); } catch (Exception ex) { - Assert.assertEquals("negative value of jib.httpTimeout", ex.getMessage()); + Assert.assertEquals("jib.httpTimeout cannot be negative: -80", ex.getMessage()); } } }