Allow configuring other strategies for finding the Google Cloud SDK. #168

briandealwis · 2016-07-14T19:16:12Z

Currently the Eclipse Tooling has wrapper classes (CloudSdkProvider and CloudSdkPrompter) for resolving the Google Cloud SDK using a user-provided location. This solution is error-prone as all code that tries to obtain a CloudSdk instance must funnel through these wrapper classes. Failures occur if there are any code paths that uses the CloudSdk.Builder directly.

This patch extracts an interface CloudSdkResolver modelled on PathResolver. The CloudSdk instance looks up additional CloudSdkResolver classes using the standard java.util.ServiceLoader mechanism. To support registering resolvers under Eclipse, where we have separate class loaders, we register the bundle using the Eclipse Buddy Policy; additional resolvers can also be contributed using OSGi fragments.

Update: Fixes #169

briandealwis · 2016-07-14T19:33:07Z

We could consider splitting out the PathResolver into separate resolvers:

Windows resolver
Environment variable resolver (GOOGLE_CLOUD_SDK_HOME)
System property resolver
We'd need to have some way to rank and order the resolvers. CloudSdkResolver could have a method to return a priority value.

briandealwis · 2016-07-15T14:19:50Z

Mentioned to João on chat:

The nice thing about these Resolver instances is that they're automatically registered.

I also wondered about turning the CloudSdkPrompter, which prompts the user to download and configure the Google Cloud SDK if required, into a last-ditch resolver:

I had wondered if we could turn the [eclipse-side] CloudSdkPrompter into the last-ditch Resolver that would prompt the user if it was running on the SWT thread and if there wasn't a some specific ThreadLocal flag to disable prompting.
Turning the [eclipse-side] Prompter into a Resolver also reduces the number of calling conventions to create / discover the SDK.

That would require some way to order the resolvers.

elharo · 2016-07-15T14:27:45Z

src/main/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdk.java

+          if (discoveredSdkPath != null) {
+            return discoveredSdkPath;
+          }
+        } catch (Exception ex) {


more specific exceptions please

This is to prevent inadvertent exceptions from breaking the flow (e.g., IOException or IllegalArgumentException). Perhaps I should just trap Throwable.

Catching java.lang.Exception is highly discouraged. (See Bloch, Effective Java). Catching Throwable is worse. In some cases it's OK to catch RuntimeException, but I'm not sure this is one of them. Usually you only want to do that when you don't know what code might be invoked. Here we should only catch what getCloudSdkPath might plausibly throw.

You're right, it should be RuntimeException. An exception here indicates a programming error in and should be treated as if the code had returned null — it didn't find an SDK in some other location.

joaoandremartins · 2016-07-15T20:25:55Z

@briandealwis I noticed you assigned this issue to me. Did you just want me to review this PR or do you expect me to follow up on your work here?

briandealwis · 2016-07-16T04:52:48Z

@joaoandremartins to review, please — I've done the follow-on work in GoogleCloudPlatform/google-cloud-eclipse#362 (to be modified once GoogleCloudPlatform/google-cloud-eclipse#335 lands)

For example, from a user-supplied preference setting.

joaoandremartins · 2016-07-19T16:01:03Z

@briandealwis GoogleCloudPlatform/google-cloud-eclipse#335 landed, let me know when this is ready for another review.

briandealwis · 2016-07-19T16:17:13Z

Please do @joaoandremartins — PTAL. It's now a priority because of gcloud-eclipse-tools#335 as you cut over CloudSdkProvider calls to Cloudsdk.Builder.build(), which breaks me ;-)

loosebazooka · 2016-07-19T16:37:38Z

src/main/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdk.java

+  /**
+   * Compare two {@link CloudSdkResolver} instances by their rank.
+   */
+  private static class ResolverComparator implements Comparator<CloudSdkResolver>, Serializable {


why does this need to be serializable?

Good question but Findbugs failed the build without it :-S

we should remove that check from our FindBugs config. It's not relevant to us.

http://findbugs.sourceforge.net/bugDescriptions.html#SE_COMPARATOR_SHOULD_BE_SERIALIZABLE

joaoandremartins · 2016-07-19T17:44:07Z

Besides the resolver order issue, LGTM.

briandealwis · 2016-07-19T19:25:28Z

@joaoandremartins Besides the resolver order issue, LGTM.

I addressed the ordering issue in d6e67b6 — do you mean the Serializable thingy?

joaoandremartins · 2016-07-19T19:37:53Z

Ah, didn't see that.
LGTM

…LIZABLE

This reverts commit 507d587.

…168)

…nes are "v2-alpha" only (#168) * Fixes #167 - Update parameter javadocs in RunMojo to indicate which ones are v2-alpha only * reformat javadoc to make Checkstyle happy

googlebot added the cla: yes label Jul 14, 2016

briandealwis assigned joaoandremartins Jul 14, 2016

briandealwis mentioned this pull request Jul 14, 2016

Remove CloudSdkProvider in favour of new CloudSdkResolver strategy GoogleCloudPlatform/google-cloud-eclipse#362

Merged

elharo reviewed Jul 15, 2016
View reviewed changes

briandealwis added 5 commits July 18, 2016 23:20

Allow configuring other strategies for finding the Google Cloud SDK.

084952d

For example, from a user-supplied preference setting.

Add a rank for CloudSdkResolver to allow sorting resolvers.

d6e67b6

Make checkstyle happy

5e2a40b

Make findbugs happy

507d587

Only catch RuntimeException when processing resolvers

955f927

briandealwis force-pushed the servicelocator branch from 1b93b73 to 955f927 Compare July 19, 2016 03:26

loosebazooka reviewed Jul 19, 2016
View reviewed changes

briandealwis mentioned this pull request Jul 19, 2016

Restore CloudSdkProvider#createBuilder() GoogleCloudPlatform/google-cloud-eclipse#401

Merged

briandealwis added 2 commits July 19, 2016 15:54

Instruct Findbugs to not complain about SE_COMPARATOR_SHOULD_BE_SERIA…

2ec64af

…LIZABLE

Revert "Make findbugs happy"

f23a177

This reverts commit 507d587.

briandealwis merged commit 4bdcb76 into master Jul 19, 2016

briandealwis deleted the servicelocator branch July 19, 2016 20:04

JoeWang1127 pushed a commit that referenced this pull request Nov 29, 2023

Allow configuring other strategies for finding the Google Cloud SDK (#…

b458a43

…168)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuring other strategies for finding the Google Cloud SDK. #168

Allow configuring other strategies for finding the Google Cloud SDK. #168

briandealwis commented Jul 14, 2016 •

edited

Loading

briandealwis commented Jul 14, 2016

briandealwis commented Jul 15, 2016

elharo Jul 15, 2016

briandealwis Jul 16, 2016

elharo Jul 16, 2016 •

edited

Loading

briandealwis Jul 19, 2016

joaoandremartins commented Jul 15, 2016

briandealwis commented Jul 16, 2016

joaoandremartins commented Jul 19, 2016

briandealwis commented Jul 19, 2016

loosebazooka Jul 19, 2016

briandealwis Jul 19, 2016

elharo Jul 19, 2016

joaoandremartins commented Jul 19, 2016

briandealwis commented Jul 19, 2016 •

edited

Loading

joaoandremartins commented Jul 19, 2016

Allow configuring other strategies for finding the Google Cloud SDK. #168

Allow configuring other strategies for finding the Google Cloud SDK. #168

Conversation

briandealwis commented Jul 14, 2016 • edited Loading

briandealwis commented Jul 14, 2016

briandealwis commented Jul 15, 2016

elharo Jul 15, 2016

Choose a reason for hiding this comment

briandealwis Jul 16, 2016

Choose a reason for hiding this comment

elharo Jul 16, 2016 • edited Loading

Choose a reason for hiding this comment

briandealwis Jul 19, 2016

Choose a reason for hiding this comment

joaoandremartins commented Jul 15, 2016

briandealwis commented Jul 16, 2016

joaoandremartins commented Jul 19, 2016

briandealwis commented Jul 19, 2016

loosebazooka Jul 19, 2016

Choose a reason for hiding this comment

briandealwis Jul 19, 2016

Choose a reason for hiding this comment

elharo Jul 19, 2016

Choose a reason for hiding this comment

joaoandremartins commented Jul 19, 2016

briandealwis commented Jul 19, 2016 • edited Loading

joaoandremartins commented Jul 19, 2016

briandealwis commented Jul 14, 2016 •

edited

Loading

elharo Jul 16, 2016 •

edited

Loading

briandealwis commented Jul 19, 2016 •

edited

Loading