Update to use ComputeClient from gcp-plugin-core. #140
Conversation
| cloud.getClient().terminateInstance(cloud.getProjectId(), remote.getZone(), instanceName); | ||
| cloud | ||
| .getClient() | ||
| .terminateInstanceAsync(cloud.getProjectId(), remote.getZone(), instanceName); |
There was a problem hiding this comment.
This changes the logic from a synchronous action to an async one, without verifying the termination was successful. Suggest either using a synchronous method, or else capturing the async operation and blocking/verifying the result.
There was a problem hiding this comment.
This was actually asynchronous previously, see https://github.com/jenkinsci/google-compute-engine-plugin/pull/140/files#diff-3a1816b899650c36d00ea7935022fe36L362 . However, this information wasn't included in the method names of the original client which is why the name has been changed in the gcp-plugin-core client.
src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineInstance.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/jenkins/plugins/computeengine/client/ClientUtil.java
Outdated
Show resolved
Hide resolved
| private static final String APPLICATION_NAME = "jenkins-google-compute-plugin"; | ||
|
|
||
| /** | ||
| * Creates a {@link ClientFactory} for generating the GCP api clients. |
src/main/java/com/google/jenkins/plugins/computeengine/client/ClientUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/jenkins/plugins/computeengine/client/ClientUtil.java
Outdated
Show resolved
Hide resolved
| @@ -49,6 +48,32 @@ | |||
| @RunWith(MockitoJUnitRunner.class) | |||
| public class ComputeEngineCloudTest { | |||
|
|
|||
| public static final PrivateKey PRIVATE_KEY; | |||
There was a problem hiding this comment.
Do these need to be public? Also, please remove the extra '\n' on line 50.
There was a problem hiding this comment.
No, they were public when these values were shared between ClientFactoryTest.java and this file, now that they are isolated to this file I've fixed that. And removed the extra '\n'
| @@ -141,7 +142,10 @@ private static String loadCredentialsString() { | |||
| } | |||
|
|
|||
| static Credentials initCredentials(JenkinsRule r) throws Exception { | |||
| ServiceAccountConfig sac = new StringJsonServiceAccountConfig(CREDENTIALS); | |||
| SecretBytes bytes = SecretBytes.fromBytes(CREDENTIALS.getBytes(StandardCharsets.UTF_8)); | |||
There was a problem hiding this comment.
This logic seems to be repeated in a few places throughout the code. Does it make sense to consolidate?
|
Suggest also adding a similar note to the README about OAuth v0.9 compatibility. |
|
I love any PR with a -1392 delta ;) |
…om this and the previous pull request to changelog, and update snapshot version to 4.0.0
Depends on: