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

Adds CloudSdk parameter for setting structured logs value #426

Merged
merged 6 commits into from
Jun 26, 2017
Merged

Adds CloudSdk parameter for setting structured logs value #426

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a1e5deb
Adds CloudSdk parameters for setting structured logs value
etanshaul Jun 20, 2017
c66ce8b
fix env var name
etanshaul Jun 21, 2017
04ecfbd
javadoc
etanshaul Jun 21, 2017
747fdb6
bump min version requirement
etanshaul Jun 21, 2017
1a46a29
unit tests for gcloud command environment variables
etanshaul Jun 21, 2017
1c83d0b
add @Nullable
etanshaul Jun 23, 2017
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
48 changes: 36 additions & 12 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,7 +59,7 @@
*/
public class CloudSdk {

public static final CloudSdkVersion MINIMUM_VERSION = new CloudSdkVersion("145.0.0");
public static final CloudSdkVersion MINIMUM_VERSION = new CloudSdkVersion("159.0.0");

private static final Logger logger = Logger.getLogger(CloudSdk.class.getName());
private static final Joiner WHITESPACE_JOINER = Joiner.on(" ");
Expand All @@ -79,22 +79,26 @@ public class CloudSdk {
private final ProcessRunner processRunner;
private final String appCommandMetricsEnvironment;
private final String appCommandMetricsEnvironmentVersion;
@Nullable
private final File appCommandCredentialFile;
private final String appCommandOutputFormat;
private final String appCommandShowStructuredLogs;
private final WaitingProcessOutputLineListener runDevAppServerWaitListener;

private CloudSdk(Path sdkPath, Path javaHomePath, String appCommandMetricsEnvironment,
String appCommandMetricsEnvironmentVersion,
private CloudSdk(Path sdkPath, Path javaHomePath,
@Nullable String appCommandMetricsEnvironment,
@Nullable String appCommandMetricsEnvironmentVersion,
@Nullable File appCommandCredentialFile,
String appCommandOutputFormat, ProcessRunner processRunner,
@Nullable String appCommandOutputFormat,
@Nullable String appCommandShowStructuredLogs,
ProcessRunner processRunner,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this Nullable? I don't see a null check in the constructor. Ditto sdkPath and javaHomePath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they are not nullable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually they may be nullable depending on which command is invoked..

Copy link
Contributor Author

@etanshaul etanshaul Jun 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@elharo. PTAL. I added an @nullable to javaHomePath as that is the only one of those that is truly nullable.

WaitingProcessOutputLineListener runDevAppServerWaitListener) {
this.sdkPath = sdkPath;
this.javaHomePath = javaHomePath;
this.appCommandMetricsEnvironment = appCommandMetricsEnvironment;
this.appCommandMetricsEnvironmentVersion = appCommandMetricsEnvironmentVersion;
this.appCommandCredentialFile = appCommandCredentialFile;
this.appCommandOutputFormat = appCommandOutputFormat;
this.appCommandShowStructuredLogs = appCommandShowStructuredLogs;
this.processRunner = processRunner;
this.runDevAppServerWaitListener = runDevAppServerWaitListener;

Expand Down Expand Up @@ -165,9 +169,20 @@ private void runGcloudCommand(List<String> args, File workingDirectory, String..
command.addAll(args);
command.addAll(GcloudArgs.get("format", appCommandOutputFormat));

Map<String, String> environment = Maps.newHashMap();
if (appCommandCredentialFile != null) {
command.addAll(GcloudArgs.get("credential-file-override", appCommandCredentialFile));
}

logCommand(command);
processRunner.setEnvironment(getGcloudCommandEnvironment());
processRunner.setWorkingDirectory(workingDirectory);
processRunner.run(command.toArray(new String[command.size()]));
}

@VisibleForTesting
Map<String, String> getGcloudCommandEnvironment() {
Map<String, String> environment = Maps.newHashMap();
if (appCommandCredentialFile != null) {
environment.put("CLOUDSDK_APP_USE_GSUTIL", "0");
}
if (appCommandMetricsEnvironment != null) {
Expand All @@ -176,6 +191,9 @@ private void runGcloudCommand(List<String> args, File workingDirectory, String..
if (appCommandMetricsEnvironmentVersion != null) {
environment.put("CLOUDSDK_METRICS_ENVIRONMENT_VERSION", appCommandMetricsEnvironmentVersion);
}
if (appCommandShowStructuredLogs != null) {
environment.put("CLOUDSDK_CORE_SHOW_STRUCTURED_LOGS", appCommandShowStructuredLogs);
}
// This is to ensure IDE credentials get correctly passed to the gcloud commands, in Windows.
// It's a temporary workaround until a fix is released.
// https://github.com/GoogleCloudPlatform/google-cloud-intellij/issues/985
Expand All @@ -185,10 +203,7 @@ private void runGcloudCommand(List<String> args, File workingDirectory, String..

environment.put("CLOUDSDK_CORE_DISABLE_PROMPTS", "1");

logCommand(command);
processRunner.setEnvironment(environment);
processRunner.setWorkingDirectory(workingDirectory);
processRunner.run(command.toArray(new String[command.size()]));
return environment;
}

// Runs a gcloud command synchronously, with a new ProcessRunner. This method is intended to be
Expand Down Expand Up @@ -525,9 +540,9 @@ public static class Builder {
private Path sdkPath;
private String appCommandMetricsEnvironment;
private String appCommandMetricsEnvironmentVersion;
@Nullable
private File appCommandCredentialFile;
private String appCommandOutputFormat;
private String appCommandShowStructuredLogs;
private boolean async = false;
private List<ProcessOutputLineListener> stdOutLineListeners = new ArrayList<>();
private List<ProcessOutputLineListener> stdErrLineListeners = new ArrayList<>();
Expand Down Expand Up @@ -584,6 +599,15 @@ public Builder appCommandOutputFormat(String appCommandOutputFormat) {
return this;
}

/**
* Sets structured json logs for the stderr output. Supported values include 'never' (default),
* 'always', 'terminal', etc.
*/
public Builder appCommandShowStructuredLogs(String appCommandShowStructuredLogs) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Name is unwieldy. Maybe just "useStructuredLogs"?

Copy link
Contributor Author

@etanshaul etanshaul Jun 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

the name mimics the gcloud config var name show_structured_logs. This is the patter we've been using here for the other parameters. But of course this can be debated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree the name isn't the best though; useStructuredLogs doesn't help much though because if you look at the values it accepts, the name should be something like setStructuredLogsScope or something like that

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that, I agree that "show" beats "use". However, I still think "appCommand" is unnecessary.

Copy link
Contributor Author

@etanshaul etanshaul Jun 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

the appCommand prefix, while ugly, is to show clearly that the scope of the parameter is for gcloud command execution (e.g. CloudSdk#runAppCommand as opposed to a dev server command or appcfg etc. which are also executed from the CloudSdk class

I'm not denying though that this whole thing could be refactored to avoid this, but I don't think this PR is the place for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually it looks @loosebazooka has some code in the experimental package that addresses all of this.

this.appCommandShowStructuredLogs = appCommandShowStructuredLogs;
return this;
}

/**
* Whether to run commands asynchronously.
*/
Expand Down Expand Up @@ -713,7 +737,7 @@ public CloudSdk build() {

return new CloudSdk(sdkPath, javaHomePath, appCommandMetricsEnvironment,
appCommandMetricsEnvironmentVersion, appCommandCredentialFile, appCommandOutputFormat,
processRunner, runDevAppServerWaitListener);
appCommandShowStructuredLogs, processRunner, runDevAppServerWaitListener);
}

/**
Expand Down
53 changes: 35 additions & 18 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 @@ -16,24 +16,6 @@

package com.google.cloud.tools.appengine.cloudsdk;

import com.google.cloud.tools.appengine.cloudsdk.CloudSdk.Builder;
import com.google.cloud.tools.appengine.cloudsdk.process.ProcessOutputLineListener;
import com.google.common.io.Files;

import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.junit.MockitoJUnitRunner;

import java.io.IOException;
import java.nio.charset.Charset;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.List;

import static org.hamcrest.core.AnyOf.anyOf;
import static org.hamcrest.core.Is.is;
import static org.hamcrest.core.StringEndsWith.endsWith;
Expand All @@ -42,8 +24,27 @@
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.fail;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import com.google.cloud.tools.appengine.cloudsdk.CloudSdk.Builder;
import com.google.cloud.tools.appengine.cloudsdk.process.ProcessOutputLineListener;
import com.google.common.io.Files;
import java.io.File;
import java.io.IOException;
import java.nio.charset.Charset;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.junit.MockitoJUnitRunner;

/**
* Unit tests for {@link CloudSdk}.
*/
Expand Down Expand Up @@ -239,4 +240,20 @@ public void testGetJavaBinary() {
System.getProperty("os.name").contains("Windows") ? "java.exe" : "java").toAbsolutePath(),
sdk.getJavaExecutablePath());
}

@Test
public void testGcloudCommandEnvironment() {
builder.appCommandShowStructuredLogs("always");
builder.appCommandCredentialFile(mock(File.class));
builder.appCommandMetricsEnvironment("intellij");
builder.appCommandMetricsEnvironmentVersion("99");
CloudSdk sdk = builder.build();

Map<String, String> env = sdk.getGcloudCommandEnvironment();
assertEquals("0", env.get("CLOUDSDK_APP_USE_GSUTIL"));
assertEquals("always", env.get("CLOUDSDK_CORE_SHOW_STRUCTURED_LOGS"));
assertEquals("intellij", env.get("CLOUDSDK_METRICS_ENVIRONMENT"));
assertEquals("99", env.get("CLOUDSDK_METRICS_ENVIRONMENT_VERSION"));
assertEquals("1", env.get("CLOUDSDK_CORE_DISABLE_PROMPTS"));
}
}