-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
notice when app engine java components aren't installed #207
Changes from all commits
a90fcf3
3c470fd
301fcad
585f1ca
19503f7
a78d87b
34580ab
29147b0
b776b12
c70ea33
7d5e3a2
e9083c2
03f93f9
d1904ae
e21364a
b6fc0ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
package com.google.cloud.tools.appengine.cloudsdk; | ||
|
||
import com.google.cloud.tools.appengine.api.AppEngineException; | ||
|
||
/** | ||
* User needs to run <samp>gcloud components install app-engine-java</samp>. | ||
*/ | ||
public class AppEngineComponentsNotInstalledException extends AppEngineException { | ||
|
||
AppEngineComponentsNotInstalledException(String message) { | ||
super(message); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,11 @@ | |
import com.google.common.collect.Lists; | ||
import com.google.common.collect.Maps; | ||
|
||
import org.json.JSONArray; | ||
import org.json.JSONException; | ||
import org.json.JSONObject; | ||
import org.json.JSONTokener; | ||
|
||
import java.io.File; | ||
import java.nio.file.Files; | ||
import java.nio.file.InvalidPathException; | ||
|
@@ -70,22 +75,22 @@ public class CloudSdk { | |
@Nullable | ||
private final File appCommandCredentialFile; | ||
private final String appCommandOutputFormat; | ||
private final WaitingProcessOutputLineListener runDevAppServerWaitListener; | ||
private final WaitingProcessOutputLineListener outputLineListener; | ||
|
||
private CloudSdk(Path sdkPath, | ||
String appCommandMetricsEnvironment, | ||
String appCommandMetricsEnvironmentVersion, | ||
@Nullable File appCommandCredentialFile, | ||
String appCommandOutputFormat, | ||
ProcessRunner processRunner, | ||
WaitingProcessOutputLineListener runDevAppServerWaitListener) { | ||
WaitingProcessOutputLineListener outputLineListener) { | ||
this.sdkPath = sdkPath; | ||
this.appCommandMetricsEnvironment = appCommandMetricsEnvironment; | ||
this.appCommandMetricsEnvironmentVersion = appCommandMetricsEnvironmentVersion; | ||
this.appCommandCredentialFile = appCommandCredentialFile; | ||
this.appCommandOutputFormat = appCommandOutputFormat; | ||
this.processRunner = processRunner; | ||
this.runDevAppServerWaitListener = runDevAppServerWaitListener; | ||
this.outputLineListener = outputLineListener; | ||
|
||
// Populate jar locations. | ||
// TODO(joaomartins): Consider case where SDK doesn't contain these jars. Only App Engine | ||
|
@@ -100,15 +105,14 @@ private CloudSdk(Path sdkPath, | |
/** | ||
* Uses the process runner to execute the gcloud app command with the provided arguments. | ||
* | ||
* @param args The arguments to pass to "gcloud app" command. | ||
* @param args the arguments to pass to "gcloud app" command | ||
*/ | ||
public void runAppCommand(List<String> args) throws ProcessRunnerException { | ||
List<String> command = new ArrayList<>(); | ||
command.add(getGCloudPath().toString()); | ||
command.add("app"); | ||
command.addAll(args); | ||
|
||
command.add("--quiet"); | ||
command.addAll(GcloudArgs.get("format", appCommandOutputFormat)); | ||
|
||
Map<String, String> environment = Maps.newHashMap(); | ||
|
@@ -125,6 +129,38 @@ public void runAppCommand(List<String> args) throws ProcessRunnerException { | |
logCommand(command); | ||
processRunner.setEnvironment(environment); | ||
processRunner.run(command.toArray(new String[command.size()])); | ||
processRunner.run(command.toArray(new String[command.size()])); | ||
} | ||
|
||
/** | ||
* Checks whether the specified component is installed in the local environment. | ||
* | ||
* @return true iff the specified component is installed in the local environment | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. {@code true} |
||
*/ | ||
public boolean isComponentInstalled(String id) throws ProcessRunnerException { | ||
List<String> command = new ArrayList<>(); | ||
command.add(getGCloudPath().toString()); | ||
command.add("components"); | ||
command.add("list"); | ||
command.add("--format=json"); | ||
command.add("--filter=id:" + id); | ||
|
||
String json = processRunner.runSynchronously(command.toArray(new String[command.size()])); | ||
|
||
try { | ||
JSONTokener tokener = new JSONTokener(json); | ||
JSONArray array = new JSONArray(tokener); | ||
if (array.length() == 0) { | ||
return false; | ||
} | ||
JSONObject object = array.getJSONObject(0); | ||
JSONObject state = object.getJSONObject("state"); | ||
String name = state.getString("name"); | ||
return "Installed".equals(name); | ||
} catch (JSONException | NullPointerException ex) { | ||
throw new AppEngineException( | ||
"Could not determine whether App Engine Java component is installed", ex); | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -149,8 +185,8 @@ public void runDevAppServerCommand(List<String> args) throws ProcessRunnerExcept | |
processRunner.run(command.toArray(new String[command.size()])); | ||
|
||
// wait for start if configured | ||
if (runDevAppServerWaitListener != null) { | ||
runDevAppServerWaitListener.await(); | ||
if (outputLineListener != null) { | ||
outputLineListener.await(); | ||
} | ||
} | ||
|
||
|
@@ -233,7 +269,7 @@ public Path getJarPath(String jarName) { | |
/** | ||
* Checks whether the configured Cloud SDK Path is valid. | ||
* | ||
* @throws AppEngineException when there is a validation error. | ||
* @throws AppEngineException when there is a validation error | ||
*/ | ||
public void validate() throws AppEngineException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if validate() expects errors why not just return the issues rather than throw them? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can do that. It is a breaking API change. |
||
if (sdkPath == null) { | ||
|
@@ -262,11 +298,20 @@ public void validate() throws AppEngineException { | |
"Validation Error: Java Tools jar location '" | ||
+ JAR_LOCATIONS.get(JAVA_TOOLS_JAR) + "' is not a file."); | ||
} | ||
try { | ||
if (!isComponentInstalled("app-engine-java")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't the block above that checks the existence of JAVA_TOOLS_JAR sufficient to tell if the component is installed? I'm still not a fan of doing this expensive check using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 to @meltsufin's comment: I tried running There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we use (or better yet, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've just tried running There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The practice of depending on the Cloud SDKs internal directory structure is not OK with the Cloud SDK team. They reserve the right to break this at their discretion. We need to move away from depending on incidental files distributed with the SDK and moving to using their api/cli for this check is part of that. This is also why we're moving to using Maven central as the source for classpath deps. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's true that the Cloud SDK reserves the right to move the files, but until we drop the dependency on AppCfg in the app-engine-java component in the Cloud SDK, we can do the file-based check without making anything more fragile in the library. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Staging is in the process of being distributed to Maven Central. We should On Thu, Sep 8, 2016 at 12:46 PM, meltsufin [email protected] wrote:
|
||
throw new AppEngineComponentsNotInstalledException( | ||
"Validation Error: App Engine Java component not installed"); | ||
} | ||
} catch (ProcessRunnerException ex) { | ||
throw new AppEngineException( | ||
"Could not determine whether App Engine Java component is installed", ex); | ||
} | ||
} | ||
|
||
@VisibleForTesting | ||
WaitingProcessOutputLineListener getRunDevAppServerWaitListener() { | ||
return runDevAppServerWaitListener; | ||
return outputLineListener; | ||
} | ||
|
||
public static class Builder { | ||
|
@@ -528,4 +573,5 @@ public int compare(CloudSdkResolver o1, CloudSdkResolver o2) { | |
} | ||
|
||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
/* | ||
* Copyright 2016 Google Inc. | ||
* | ||
* 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.appengine.cloudsdk.internal.process; | ||
|
||
import com.google.cloud.tools.appengine.cloudsdk.process.ProcessOutputLineListener; | ||
|
||
class AccumulatingLineListener implements ProcessOutputLineListener { | ||
|
||
private StringBuilder output = new StringBuilder(); | ||
|
||
@Override | ||
public void onOutputLine(String line) { | ||
output.append(line + "\n"); | ||
} | ||
|
||
String getOutput() { | ||
return output.toString(); | ||
} | ||
|
||
public void clear() { | ||
output = new StringBuilder(); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
package com.google.cloud.tools.appengine.cloudsdk; | ||
|
||
import static org.junit.Assert.assertFalse; | ||
import static org.junit.Assert.assertTrue; | ||
|
||
import java.nio.file.Files; | ||
|
||
import org.junit.Test; | ||
|
||
import com.google.cloud.tools.appengine.cloudsdk.internal.process.ProcessRunnerException; | ||
|
||
/** | ||
* Integration tests for {@link CloudSdk} that require an installed CloudSdk instance. | ||
*/ | ||
public class CloudSdkEnvironmentTest { | ||
|
||
private CloudSdk sdk = new CloudSdk.Builder().build(); | ||
|
||
@Test | ||
public void testGetSdkPath() { | ||
assertTrue(Files.exists(sdk.getSdkPath())); | ||
} | ||
|
||
@Test | ||
public void testIsComponentInstalled_true() throws ProcessRunnerException { | ||
assertTrue(sdk.isComponentInstalled("app-engine-java")); | ||
} | ||
|
||
@Test | ||
public void testIsComponentInstalled_False() throws ProcessRunnerException { | ||
assertFalse(sdk.isComponentInstalled("no-such-component")); | ||
} | ||
|
||
@Test | ||
public void testIsComponentInstalled_sequential() throws ProcessRunnerException { | ||
assertTrue(sdk.isComponentInstalled("app-engine-java")); | ||
assertFalse(sdk.isComponentInstalled("no-such-component")); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't AppEngineException for errors caused by gcloud invocations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also I haven't checked the styleguide recently, is the javadoc no longer required for all public classes?=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added Javadoc. This seems close enough to AppEngineException to justify subclassing. I can change to a different exception if you like, but that would require clients to update.