Skip to content
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.

Sign up for GitHub

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 #206

Closed
wants to merge 19 commits into from
Closed

notice when app engine java components aren't installed #206

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a90fcf3
@Override
Sep 7, 2016
3c470fd
notice when app engine java components aren't installed
Sep 7, 2016
7c7406e
Merge branch 'master' into i165B
elharo Sep 7, 2016
301fcad
Merge branch 'master' into i165
Sep 7, 2016
585f1ca
use gcloud components list to look for app engine java components
Sep 8, 2016
19503f7
Merge branch 'i165' into i165b
Sep 8, 2016
a78d87b
use gcloud components list to look for app engine java components
Sep 8, 2016
34580ab
use gcloud components list to look for app engine java components
Sep 8, 2016
29147b0
fix up tests
Sep 8, 2016
b776b12
parse JSON response
Sep 8, 2016
c70ea33
gcloud returns JSON arrays
Sep 8, 2016
7d5e3a2
add exception javadoc
Sep 8, 2016
e9083c2
push stdOut collection into ProcessRunner
Sep 8, 2016
03f93f9
Merge branch 'master' into i165b
Sep 8, 2016
d1904ae
checkstyle fixes
Sep 8, 2016
e21364a
install cloud sdk for integration tests
Sep 9, 2016
b6fc0ed
add gcloud to path
Sep 9, 2016
97da3d0
look for different file
Sep 12, 2016
124642d
Merge branch 'i165B' of github.com:GoogleCloudPlatform/appengine-plug…
Sep 12, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions ...a/com/google/cloud/tools/appengine/cloudsdk/AppEngineComponentsNotInstalledException.java
View file
Open in desktop
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);
}

}
10 changes: 10 additions & 0 deletions src/main/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdk.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public class CloudSdk {
private static final String DEV_APPSERVER_PY = "bin/dev_appserver.py";
private static final String JAVA_APPENGINE_SDK_PATH =
"platform/google_appengine/google/appengine/tools/java/lib";
private static final String JAVA_COMPONENTS_PATH = "platform/gcd/.appengine";
Copy link
Member

@meltsufin meltsufin Sep 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it sufficient to just check for the existence of JAVA_APPENGINE_SDK_PATH?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that seems to exist regardless

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably check with the Cloud SDK team to see what they recommend using.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would executing and parsing a gcloud components list be overkill?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markpell, @znewman01 can you comment on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to know whether or not the user ran:

gcloud components install app-engine-java

We've seen multiple user reports that trace back to not having done this.
This directory appears to be added/removed when the user installs/removes the component.

Copy link
Member

@briandealwis briandealwis Sep 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW: although my installation is out of date, I did have app-engine-java, but just updated and I don't have anything in platform/gcd.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since what we're actually using is the appengine-tools-api.jar, we should probably just check for its existence.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So again, this is just checking for the wrong thing. That directory is for the legacy datastore emulator which has nothing to do with Java or any Java components.

How far are we from having the requiring staging code as separate artifacts on maven central? I don't think it's appropriate for the plugin to be grabbing individual jars from the SDK as this is not a public interface.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ludoch who would know when staging is going to make it to maven central.

To summarize what's being discussed:

For component detection we should be calling gcloud components list and not digging into the cloud SDK packaged files.

The staging question is not related to this CL but we are working on moving as many deps as possible away from using Cloud SDK paths.

private static final String JAVA_TOOLS_JAR = "appengine-tools-api.jar";
private static final Map<String, Path> JAR_LOCATIONS = new HashMap<>();
private static final String WINDOWS_BUNDLED_PYTHON = "platform/bundledpython/python.exe";
Expand Down Expand Up @@ -181,6 +182,11 @@ private void logCommand(List<String> command) {
public Path getSdkPath() {
return sdkPath;
}

// TODO does this work on windows?
Path getJavaAppEngineComponentsPath() {
return sdkPath.resolve(JAVA_COMPONENTS_PATH);
}

private Path getGCloudPath() {
String gcloud = GCLOUD;
Expand Down Expand Up @@ -262,6 +268,10 @@ public void validate() throws AppEngineException {
"Validation Error: Java Tools jar location '"
+ JAR_LOCATIONS.get(JAVA_TOOLS_JAR) + "' is not a file.");
}
if (!Files.isDirectory(getJavaAppEngineComponentsPath())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my concern about having this here is that we can support deploy without needing the java components installed. A client should be able to get configure gcloud and do a deployment without install the components,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this only applies to Standard and Compat deployments, not pure Flex.

throw new AppEngineComponentsNotInstalledException(
"Validation Error: Java App Engine components not installed");
}
}

@VisibleForTesting
Expand Down
9 changes: 7 additions & 2 deletions src/test/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdkTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,8 @@
import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.when;

import com.google.cloud.tools.appengine.cloudsdk.CloudSdk.Builder;

import com.google.cloud.tools.appengine.api.AppEngineException;
import com.google.cloud.tools.appengine.cloudsdk.CloudSdk.Builder;
import com.google.cloud.tools.appengine.cloudsdk.process.ProcessOutputLineListener;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand Down Expand Up @@ -47,6 +46,12 @@ public void testGetJavaAppEngineSdkPath() {
assertEquals(root.resolve("platform/google_appengine/google/appengine/tools/java/lib"),
builder.build().getJavaAppEngineSdkPath());
}

@Test
public void testGetJavaAppEngineComponentsPath() {
assertEquals(root.resolve("platform/gcd/.appengine"),
builder.build().getJavaAppEngineComponentsPath());
}

@Test
public void testGetJarPathJavaTools() {
Expand Down