Prompt the user if unpublished changes detected during launch by briandealwis · Pull Request #2047 · GoogleCloudPlatform/google-cloud-eclipse

briandealwis · 2017-06-09T20:08:44Z

I originally tried to detect this unpublished changes situation and tried to force publishing the changes. The code was becoming sufficiently icky that I punted and instead detect the situation and prompt the user whether to continue.

The fix uses the Platform/Debug prompting framework, which is somewhat baroque as you'll see. It uses a Status's plugin ID and error-code to lookup a status handler. We use the prompter status handler and ask it to punt to our custom stale-resources handler to prompt the user, returning a boolean.

Fixes #1832
Fixes #1897 too

briandealwis · 2017-06-09T20:09:33Z

...ine.localserver/src/com/google/cloud/tools/eclipse/appengine/localserver/messages.properties

not yet been published?

no hyphen in "recently changed".
"yet" feels redundant. I suggest deleting it completely.

elharo · 2017-06-09T20:17:55Z

plugins/com.google.cloud.tools.eclipse.appengine.localserver/plugin.xml

+         point="org.eclipse.debug.core.statusHandlers">
+      <statusHandler
+            class="com.google.cloud.tools.eclipse.appengine.localserver.ui.StaleResourcesStatusHandler"
+            code="255"


a comment for "255" might be helpful

elharo · 2017-06-09T20:19:42Z

...ine.localserver/src/com/google/cloud/tools/eclipse/appengine/localserver/messages.properties

One space after period. Is it really "Recently changed" or is it "Unsaved changes"?

"Recently changed resources have not been published yet." or "Unsaved changes have not been published."

Recently changed. These stale resources are usually from the user having saved their unsaved changes, which haven't been published yet.

Nit:

One space after period.

briandealwis · 2017-06-09T20:25:03Z

I've decided to make a few more changes.

chanseokoh · 2017-06-09T20:27:11Z

...src/com/google/cloud/tools/eclipse/appengine/localserver/ui/StaleResourcesStatusHandler.java

+  static final int STALE_RESOURCES_CODE = 255;
+
+  /** The status object to pass into the debug prompter */
+  public static final IStatus CONTINUE_LAUNCH_REQUEST = new Status(IStatus.INFO,


StatusUtil.info()?

This is a specially crafted status message; I'll add to the comment.

chanseokoh · 2017-06-09T20:28:33Z

...src/com/google/cloud/tools/eclipse/appengine/localserver/ui/StaleResourcesStatusHandler.java

+    if (source instanceof ILaunchConfiguration) {
+      ILaunchConfiguration config = (ILaunchConfiguration) source;
+      if (DebugUITools.isPrivate(config)) {
+        return Boolean.FALSE;


What does a "private" config mean? So, if it's private, does this stop launching?

From what I gather, it's used for launches that aren't visible to the user. Like Ant builds or Maven builds. We could conceivably usr launches for our staging, etc. The advantage being that it's tracked and managed by the launch info.

chanseokoh · 2017-06-09T20:32:23Z

...ls/eclipse/appengine/localserver/server/LocalAppEngineServerLaunchConfigurationDelegate.java

+   * Check if there are pending changes to be published: this is a nasty condition that can occur if
+   * the user saves changes as part of launching the server.
+   */
+  private boolean hasPendingChangesToPublish() {


I'm just wondering if this method can be called before some ResourceChangeJob is actually scheduled?

Good point. It may not… It should be safe to join on the AUTOBUILD job.

I see. However, I'm fine with the current status if that turns out too complex or not really worth.

After experimenting, we need to join on the auto build job to ensure the workspace's resource changes happen, so that we see that there are pending events for publishing.

- Change behaviour to check if there are pending changes to be published and prompt the user whether to continue

briandealwis · 2017-06-12T20:11:50Z

PTAL @elharo @chanseokoh

chanseokoh · 2017-06-12T20:23:19Z

...m/google/cloud/tools/eclipse/appengine/localserver/server/LocalAppEngineServerBehaviour.java

allPublished

chanseokoh · 2017-06-12T20:25:04Z

...m/google/cloud/tools/eclipse/appengine/localserver/server/LocalAppEngineServerBehaviour.java

for (IModule module : modules) will probably work.

chanseokoh · 2017-06-12T20:44:51Z

The test failures seem very interesting. It always fails with #1897 (cf. #1975). This may shed some light on #1897.

Dismissed until test failures are fixed.

briandealwis · 2017-06-13T18:55:08Z

testDebugLaunch(com.google.cloud.tools.eclipse.integration.appengine.DebugNativeAppEngineStandardProjectTest)  Time elapsed: 28.184 sec  <<< FAILURE!
java.lang.AssertionError: No App Engine launch found
Expected: a string containing "App Engine Standard at localhost"
     but: was "<terminated>testapp [Run on Server]"
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.junit.Assert.assertThat(Assert.java:956)
	at com.google.cloud.tools.eclipse.integration.appengine.DebugNativeAppEngineStandardProjectTest.testDebugLaunch(DebugNativeAppEngineStandardProjectTest.java:107)

I improved the test to use Hamcrest's Matchers.containsString(), and what's shown is
<terminated>testapp [Run on Server]. I reproduced it locally, and the Debug view shows:

We should remove all terminated processes, I guess.

briandealwis · 2017-06-15T13:42:49Z

PTAL @chanseokoh

Prompt the user if unpublished changes detected during launch

911889e

briandealwis requested a review from elharo June 9, 2017 20:08

googlebot added the cla: yes label Jun 9, 2017

briandealwis commented Jun 9, 2017

View reviewed changes

elharo reviewed Jun 9, 2017

View reviewed changes

chanseokoh reviewed Jun 9, 2017

View reviewed changes

briandealwis added 2 commits June 12, 2017 14:13

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

03ab310

- Configure the server as publishing-then-launch.

0fc7a39

- Change behaviour to check if there are pending changes to be published and prompt the user whether to continue

chanseokoh previously approved these changes Jun 12, 2017

View reviewed changes

briandealwis added 2 commits June 13, 2017 13:42

Review nits

23a28f8

Improve test logging

350b10f

Remove all terminated processed from Debug View

280c8a8

briandealwis mentioned this pull request Jun 13, 2017

Flaky test on Kokoro Windows: DebugNativeAppEngineStandardProjectTest.testDebugLaunch() #1897

Closed

Merge remote-tracking branch 'origin/master' into ide-target-platform

1bdef74

chanseokoh approved these changes Jun 15, 2017

View reviewed changes

briandealwis merged commit 2b18352 into master Jun 15, 2017

briandealwis deleted the i1832 branch June 15, 2017 14:37

chanseokoh mentioned this pull request Jun 15, 2017

Log debug info for #1897 #1975

Closed

Conversation

briandealwis commented Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briandealwis commented Jun 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chanseokoh Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briandealwis commented Jun 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chanseokoh commented Jun 12, 2017

Uh oh!

briandealwis commented Jun 13, 2017

Uh oh!

briandealwis commented Jun 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

briandealwis commented Jun 9, 2017 •

edited

Loading

chanseokoh Jun 9, 2017 •

edited

Loading