-
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 8 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,11 @@ | ||
package com.google.cloud.tools.appengine.cloudsdk; | ||
|
||
import com.google.cloud.tools.appengine.api.AppEngineException; | ||
|
||
public class AppEngineComponentsNotInstalledException extends AppEngineException { | ||
|
||
AppEngineComponentsNotInstalledException(String message) { | ||
super(message); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ | |
|
||
import com.google.cloud.tools.appengine.api.AppEngineException; | ||
import com.google.cloud.tools.appengine.cloudsdk.internal.args.GcloudArgs; | ||
import com.google.cloud.tools.appengine.cloudsdk.internal.process.AccumulatingLineListener; | ||
import com.google.cloud.tools.appengine.cloudsdk.internal.process.DefaultProcessRunner; | ||
import com.google.cloud.tools.appengine.cloudsdk.internal.process.ProcessRunner; | ||
import com.google.cloud.tools.appengine.cloudsdk.internal.process.ProcessRunnerException; | ||
|
@@ -70,22 +71,25 @@ public class CloudSdk { | |
@Nullable | ||
private final File appCommandCredentialFile; | ||
private final String appCommandOutputFormat; | ||
private final WaitingProcessOutputLineListener runDevAppServerWaitListener; | ||
private final WaitingProcessOutputLineListener outputLineListener; | ||
private final AccumulatingLineListener stdOutput; | ||
|
||
private CloudSdk(Path sdkPath, | ||
String appCommandMetricsEnvironment, | ||
String appCommandMetricsEnvironmentVersion, | ||
@Nullable File appCommandCredentialFile, | ||
String appCommandOutputFormat, | ||
ProcessRunner processRunner, | ||
WaitingProcessOutputLineListener runDevAppServerWaitListener) { | ||
WaitingProcessOutputLineListener outputLineListener, | ||
AccumulatingLineListener stdOutput) { | ||
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. Do we really need two kinds of listener API's? It seems unlikely the user would want to pass both? 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. They do quite different things. It might make sense to move the AccumulatingLineListener into ProcessRunner itself. It's not exposed in the public API (constructor is private) |
||
this.sdkPath = sdkPath; | ||
this.appCommandMetricsEnvironment = appCommandMetricsEnvironment; | ||
this.appCommandMetricsEnvironmentVersion = appCommandMetricsEnvironmentVersion; | ||
this.appCommandCredentialFile = appCommandCredentialFile; | ||
this.appCommandOutputFormat = appCommandOutputFormat; | ||
this.processRunner = processRunner; | ||
this.runDevAppServerWaitListener = runDevAppServerWaitListener; | ||
this.outputLineListener = outputLineListener; | ||
this.stdOutput = stdOutput; | ||
|
||
// Populate jar locations. | ||
// TODO(joaomartins): Consider case where SDK doesn't contain these jars. Only App Engine | ||
|
@@ -100,15 +104,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 +128,31 @@ 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()])); | ||
} | ||
|
||
/** | ||
* @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); | ||
|
||
if (stdOutput == null) { | ||
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 stdOutput non-null? Should we just add Preconditions not null checks to the constructor? 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. what do you think of making the output handling completely internal to this method? It doesn't seem like clients should need to think about this. 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 would love to do that. I think it might require major refactoring of the API though, but let me give it a bit more thought and see if I can crack it. 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. There is a code path along which stdOutput is not set, basically if inheritProcessOutput is true. See line 472-478. I'm not sure if we really need inheritProcessOutput though. Maybe we can drop it? |
||
throw new IllegalStateException("won't be able to read output"); | ||
} | ||
|
||
stdOutput.clear(); | ||
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 guess this class is just generally not threadsafe? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think ProcessRunner is not thread safe. |
||
processRunner.run(command.toArray(new String[command.size()])); | ||
|
||
String json = stdOutput.getOutput(); | ||
|
||
// todo: parse the JSON | ||
return json.contains("\"name\": \"Installed\""); | ||
} | ||
|
||
/** | ||
|
@@ -149,8 +177,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 +261,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 +290,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 { | ||
|
@@ -415,10 +452,14 @@ public CloudSdk build() { | |
|
||
// Verify there aren't listeners if subprocess inherits output. | ||
// If output is inherited, then listeners won't receive anything. | ||
AccumulatingLineListener stdOut = null; | ||
if (inheritProcessOutput | ||
&& (stdOutLineListeners.size() > 0 || stdErrLineListeners.size() > 0)) { | ||
throw new AppEngineException("You cannot specify subprocess output inheritance and" | ||
+ " output listeners."); | ||
} else { | ||
stdOut = new AccumulatingLineListener(); | ||
stdOutLineListeners.add(stdOut); | ||
} | ||
|
||
// Construct process runner. | ||
|
@@ -445,7 +486,7 @@ public CloudSdk build() { | |
|
||
return new CloudSdk(sdkPath, appCommandMetricsEnvironment, | ||
appCommandMetricsEnvironmentVersion, appCommandCredentialFile, appCommandOutputFormat, | ||
processRunner, runDevAppServerWaitListener); | ||
processRunner, runDevAppServerWaitListener, stdOut); | ||
} | ||
|
||
/** | ||
|
@@ -528,4 +569,5 @@ public int compare(CloudSdkResolver o1, CloudSdkResolver o2) { | |
} | ||
|
||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
/* | ||
* 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; | ||
|
||
public class AccumulatingLineListener implements ProcessOutputLineListener { | ||
|
||
private StringBuilder output = new StringBuilder(); | ||
|
||
@Override | ||
public void onOutputLine(String line) { | ||
output.append(line + "\n"); | ||
} | ||
|
||
// todo: maybe this should be a standard part of ProcessRunner API instead? | ||
public 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.