Add support for YAML files for App Engine content block #3186

briandealwis · 2018-06-28T16:50:05Z

Adds support for app.yaml and the various App Engine .yaml configuration files. This change moves support for resolving App Engine configuration files into a utility class called AppEngineConfigurationUtil; we were previously lumping these files into WEB-INF. The idea is that this class can use the project facets and language types to determine the right place for these configuration files.

But you'll notice that this AppEngineConfigurationUtil class doesn't currently do any of the special logic. I started down the path of adding special logic for App Engine standard vs flexible for determining where these configuration files should be looked up, but giving some of @ludoch's musings about new configuration files I thought it was better to support a src/main/appengine if present, and figure out the rest later. It checks if there's an src/main/appengine and tries to resolve the file there, and otherwise looks in the WEB-INF. This works for our existing projects as AES projects don't have src/main/appengine and all files are placed in WEB-INF, and AEF projects were using src/main/appengine anyways. This has a few knock-on effects:

App Engine .yaml configuration files will be found and showed when they are located in WEB-INF: this is unlikely.
AES projects with src/main/appengine will break on run or deployment since our launch and deployment processes do not currently resolving files from there. This is an unlikely to happen in practice. And such projects can easily be made to work with by mapping src/main/appengine to WEB-INF the project's Deployment Assembly.

We may want to teach our projects to place these App Engine configuration files to an alternative well-known location via the deployment assembly (e.g., WEB-INF/appengine)?

Fixes #3091
Fixes #3099

For example: project was using app.yaml, but appengine-web.xml appears

codecov · 2018-06-28T16:50:17Z

Codecov Report

Merging #3186 into master will increase coverage by 0.26%.
The diff coverage is 90.28%.

@@             Coverage Diff              @@
##             master    #3186      +/-   ##
============================================
+ Coverage     69.75%   70.02%   +0.26%     
- Complexity     2931     2963      +32     
============================================
  Files           372      373       +1     
  Lines         13422    13511      +89     
  Branches       1611     1622      +11     
============================================
+ Hits           9363     9461      +98     
+ Misses         3377     3373       -4     
+ Partials        682      677       -5

Impacted Files	Coverage Δ	Complexity Δ
...erver/ui/DatastoreIndexesUpdatedStatusHandler.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
.../ui/navigator/model/DenialOfServiceDescriptor.java	`100% <100%> (ø)`	`2 <1> (ø)`	⬇️
...rd/java8/AppEngineStandardFacetChangeListener.java	`71.83% <100%> (+0.4%)`	`15 <0> (ø)`	⬇️
...va8/AppEngineStandardJre8ProjectFacetDetector.java	`73.17% <100%> (+0.67%)`	`6 <0> (ø)`	⬇️
...ipse/appengine/localserver/server/ModuleUtils.java	`81.81% <100%> (+0.56%)`	`9 <0> (ø)`	⬇️
...acets/ui/navigator/model/TaskQueuesDescriptor.java	`100% <100%> (ø)`	`2 <1> (ø)`	⬇️
.../ui/navigator/model/DispatchRoutingDescriptor.java	`100% <100%> (ø)`	`2 <1> (ø)`	⬇️
...e/standard/java8/ConvertToJava8RuntimeHandler.java	`87.5% <100%> (+0.54%)`	`4 <0> (ø)`	⬇️
.../appengine/standard/java8/AppEngineWebBuilder.java	`74.5% <100%> (+0.5%)`	`10 <0> (ø)`	⬇️
...ne/facets/ui/navigator/AppEngineLabelProvider.java	`96% <100%> (+11.38%)`	`23 <13> (+1)`	⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa89aa3...b061eb0. Read the comment docs.

chanseokoh · 2018-06-28T19:31:44Z

...m/google/cloud/tools/eclipse/appengine/facets/ui/navigator/AppEngineContentProviderTest.java

+  }
+
+  /**
+   * Test that creating that a newly-created {@code appengine-web.xml} "overrides" the previous


"... creating a newly ..."

chanseokoh · 2018-06-28T19:36:10Z

...com/google/cloud/tools/eclipse/appengine/facets/ui/navigator/AppEngineLabelProviderTest.java

+  public void setUp() {
+    // Many of ResourceManager's methods are final; we check getImage() methods
+    // by verifying that create() is called
+    doReturn(null).when(resourceManager).create(any(DeviceResourceDescriptor.class));


AFAIK, a mock will return null for non-mocked methods by default, so I don't get why this is needed.

hmm, I'm sure there was a reason! But it doesn't seem to fail now…

chanseokoh · 2018-06-28T19:55:08Z

...rc/com/google/cloud/tools/eclipse/appengine/facets/ui/navigator/model/ModelRefreshTests.java

@@ -105,8 +105,8 @@ public void testAppEngineStandardProjectElementCreate_initial() throws AppEngine
  @Test
  public void testAppEngineStandardProjectElementCreate_staggered() throws AppEngineException {


The method name contains "Standard". There are other methods having that too.

chanseokoh · 2018-06-28T20:03:43Z

...e.facets/src/com/google/cloud/tools/eclipse/appengine/facets/AppEngineConfigurationUtil.java

+   * Create an App Engine configuration file in the appropriate location for the project.
+   *
+   * @param project the hosting project
+   * @param relativePath the file name


"relative path to the file"?

chanseokoh · 2018-06-28T20:08:14Z

...e.facets/src/com/google/cloud/tools/eclipse/appengine/facets/AppEngineConfigurationUtil.java

+  public static IFile createConfigurationFile(
+      IProject project,
+      IPath relativePath,
+      InputStream contents,


I was going to say this could just be String, but then WebProjectUtil.createFileInWebInf() takes InputStream too. Looking at the callers, they could both accept String, but I don't want to stretch this PR for that. I just think this can just be String, what about you?

It's modelled directly after IFile's create() / setContent() which takes an InputStream. But I certainly don't see anything wrong with helper methods.

chanseokoh · 2018-06-28T20:18:56Z

...e.facets/src/com/google/cloud/tools/eclipse/appengine/facets/AppEngineConfigurationUtil.java

+    IFolder appengineFolder = project.getFolder(DEFAULT_CONFIGURATION_FILE_LOCATION);
+    if (appengineFolder != null && appengineFolder.exists()) {
+      IFile destination = appengineFolder.getFile(relativePath);
+      if (destination.exists()) {


No need to call exists() twice. This could basically be

IFile destination = project.getFolder(DEFAULT_CONFIGURATION_FILE_LOCATION).getFile(relativePath);

Ditto the above: it can be with mocks.

getFolder() returning null.

Okay, that's fair. I still don't get the inconsistency (checking null for getFolder() but not for getFile(), etc.), but this is not a big deal.

Point taken: I'll add a null check here too.

You're right the null check shouldn't be necessary and I will instead fix up any tests.

But back to your original question about combining into one chain: we need to split it up as we only create or update the file providing the src/main/appengine exists. If it doesn't exist, then we put it into WEB-INF. I think it's time for me to go home: responding to the wrong comment.

chanseokoh · 2018-06-28T20:27:33Z

...src/com/google/cloud/tools/eclipse/appengine/facets/ui/navigator/AppEngineLabelProvider.java

  @VisibleForTesting
-  static String getVersionTuple(AppEngineDescriptor descriptor) throws AppEngineException {
+  static String getVersionTuple(String projectId, String projectVersion, String serviceId) {


The order seems weird, given that the tuple will be "project:service:version".

This method was a hold-over when I maintained both a AppEngineDescriptor and AppYaml instance. I can just pass in the AppEngineProjectElement.

chanseokoh · 2018-06-28T20:32:08Z

...c/com/google/cloud/tools/eclipse/appengine/facets/ui/navigator/AppEngineContentProvider.java

@@ -204,8 +209,7 @@ private void resourceChanged(IResourceChangeEvent event) {
          projectMapping.invalidate(project);
          toBeRefreshed.add(project);
        }
-      } else if (Iterables.any(
-          projectFiles, file -> file != null && "appengine-web.xml".equals(file.getName()))) {
+      } else if (anyMatch(projectFiles, Sets.newHashSet("appengine-web.xml", "app.yaml"))) {


Can we reuse AppEngineProjectElement.hasAppEngineDescriptor()?

Oh yes! I forgot about this.

chanseokoh · 2018-06-28T20:34:04Z

.../google/cloud/tools/eclipse/appengine/facets/ui/navigator/model/AppEngineProjectElement.java

+  private String projectVersion;
+  private String serviceId;
+
+  /** Return the App Engine environment type (e.g., standard, flexible). */


Looking at the code, this would be flex for the flexible and null for the standard, wouldn't it?

chanseokoh · 2018-06-28T20:37:34Z

.../google/cloud/tools/eclipse/appengine/facets/ui/navigator/model/AppEngineProjectElement.java

+  /** Return the App Engine environment type (e.g., standard, flexible). */
+  private String environmentType;
+
+  /** Return the App Engine runtime (e.g., java7, java8). */


The comment is not really correct. The values are not compatible between standard and flex, and I think we should clearly explain it. For example, for standard, the values are null and java8, and for flex, the values are java, custom, python27, etc. I haven't checked the actual runtime: value of app.yaml auto-generated from appengine-web.xml where <runtime> is not present (i.e., java7) or it is set to java8, but another point of confusion could be the possibly different values of runtime: and <runtime> after the auto-conversion.

… and here. I'm currently returning the actual values and leaving the interpretation in getStyledText(). But it's more useful to provide the right interpretation here.

Verified auto-generated app.yaml from an appengine-web.xml has runtime: java7 or runtime: java8.

…time()

chanseokoh · 2018-06-29T18:28:48Z

third_party/com.google.cloud.tools.eclipse.jdt.launching/.classpath

@@ -3,6 +3,5 @@
 	<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-1.7"/>
 	<classpathentry kind="con" path="org.eclipse.pde.core.requiredPlugins"/>
 	<classpathentry kind="src" path="src/"/>
-    <classpathentry kind="src" path="tests"/>


Just confirming (and for documentation), this is a cleanup?

Oops sorry, this shouldn't have been included. Photon now supports scoped classpath entries, and some action occasionally causes the IDE to regenerate the .classpath files.

chanseokoh · 2018-06-29T19:35:47Z

...e.facets/src/com/google/cloud/tools/eclipse/appengine/facets/AppEngineConfigurationUtil.java

+    IFolder appengineFolder = project.getFolder(DEFAULT_CONFIGURATION_FILE_LOCATION);
+    if (appengineFolder != null && appengineFolder.exists()) {
+      IFile destination = appengineFolder.getFile(relativePath);
+      if (destination.exists()) {


Okay, that's fair. I still don't get the inconsistency (checking null for getFolder() but not for getFile(), etc.), but this is not a big deal.

briandealwis · 2018-06-29T20:05:24Z

We have this resource != null && resource.exists() pattern throughout the code base. It shouldn't be necessary, but I'll fix that separately.

briandealwis added 7 commits June 23, 2018 00:22

Add support for App Engine YAML configuration files too

e8c5ab3

Separate out the App Engine configuration file resolution logic

b4c9cd6

Use JRE7 for now

fcf5e01

Flip to AppEngineConfigurationUtil

db70d77

Add support for detecting and handling new descriptor files

f200cda

For example: project was using app.yaml, but appengine-web.xml appears

Drop unnecessary IOException

e2508e3

Merge remote-tracking branch 'origin/master' into HEAD

d3ef042

googlebot added the cla: yes label Jun 28, 2018

chanseokoh reviewed Jun 28, 2018

View reviewed changes

briandealwis added 2 commits June 29, 2018 00:17

Review

2e17da9

Add tests for AppEngineProjectElement#getEnvironmentType() and getRun…

e7c2f71

…time()

chanseokoh reviewed Jun 29, 2018

View reviewed changes

restore .classpath

b061eb0

chanseokoh approved these changes Jun 29, 2018

View reviewed changes

briandealwis merged commit f909706 into master Jun 29, 2018

briandealwis deleted the i3091 branch June 29, 2018 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for YAML files for App Engine content block #3186

Add support for YAML files for App Engine content block #3186

briandealwis commented Jun 28, 2018

codecov bot commented Jun 28, 2018 •

edited

Loading

chanseokoh Jun 28, 2018

chanseokoh Jun 28, 2018

briandealwis Jun 29, 2018

chanseokoh Jun 28, 2018

chanseokoh Jun 28, 2018

chanseokoh Jun 28, 2018

briandealwis Jun 29, 2018

chanseokoh Jun 28, 2018

briandealwis Jun 29, 2018

chanseokoh Jun 29, 2018

briandealwis Jun 29, 2018

chanseokoh Jun 29, 2018

briandealwis Jun 29, 2018

briandealwis Jun 29, 2018 •

edited

Loading

chanseokoh Jun 28, 2018

briandealwis Jun 29, 2018

chanseokoh Jun 28, 2018

briandealwis Jun 29, 2018

chanseokoh Jun 28, 2018

chanseokoh Jun 28, 2018

briandealwis Jun 29, 2018

briandealwis Jun 29, 2018

chanseokoh Jun 29, 2018

briandealwis Jun 29, 2018

chanseokoh Jun 29, 2018

briandealwis commented Jun 29, 2018

		@@ -105,8 +105,8 @@ public void testAppEngineStandardProjectElementCreate_initial() throws AppEngine
		@Test
		public void testAppEngineStandardProjectElementCreate_staggered() throws AppEngineException {

Add support for YAML files for App Engine content block #3186

Add support for YAML files for App Engine content block #3186

Conversation

briandealwis commented Jun 28, 2018

codecov bot commented Jun 28, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briandealwis Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briandealwis commented Jun 29, 2018

codecov bot commented Jun 28, 2018 •

edited

Loading

briandealwis Jun 29, 2018 •

edited

Loading