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.

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

Add a debug flag that enables wire logging for git http requests #1125

Merged
merged 5 commits into from
Jan 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions dockerfile-image-update/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,16 @@
<artifactId>bucket4j-core</artifactId>
<version>8.1.1</version>
</dependency>
<dependency>
<groupId>com.squareup.okhttp3</groupId>
<artifactId>okhttp</artifactId>
<version>4.9.1</version>
</dependency>
<dependency>
<groupId>com.squareup.okhttp3</groupId>
<artifactId>logging-interceptor</artifactId>
<version>4.9.1</version>
</dependency>
Comment on lines +147 to +156
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For instrumenting the client with logging functionality.

</dependencies>

<build>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,21 +10,29 @@

import com.google.common.reflect.ClassPath;
import com.salesforce.dockerfileimageupdate.subcommands.ExecutableWithNamespace;
import com.salesforce.dockerfileimageupdate.utils.Constants;
import com.salesforce.dockerfileimageupdate.utils.DockerfileGitHubUtil;
import com.salesforce.dockerfileimageupdate.utils.GitHubUtil;
import net.sourceforge.argparse4j.ArgumentParsers;
import net.sourceforge.argparse4j.impl.Arguments;
import net.sourceforge.argparse4j.inf.*;
import okhttp3.OkHttpClient;
import okhttp3.logging.HttpLoggingInterceptor;

import org.kohsuke.github.GitHub;
import org.kohsuke.github.GitHubBuilder;
import org.kohsuke.github.extras.okhttp3.OkHttpGitHubConnector;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.IOException;
import java.util.Comparator;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.TreeSet;
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.Supplier;
import java.util.stream.Collectors;
import static com.salesforce.dockerfileimageupdate.utils.Constants.*;

Expand All @@ -45,8 +53,8 @@
Namespace ns = handleArguments(parser, args);
if (ns == null)
System.exit(1);
Class<?> runClass = loadCommand(allClasses, ns.get(Constants.COMMAND));
DockerfileGitHubUtil dockerfileGitHubUtil = initializeDockerfileGithubUtil(ns.get(Constants.GIT_API));
Class<?> runClass = loadCommand(allClasses, ns.get(COMMAND));
DockerfileGitHubUtil dockerfileGitHubUtil = initializeDockerfileGithubUtil(gitApiUrl(ns), gitApiToken(), ns.getBoolean(DEBUG));

Check warning on line 57 in dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/CommandLine.java

View check run for this annotation

Codecov / codecov/patch

dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/CommandLine.java#L56-L57

Added lines #L56 - L57 were not covered by tests

/* Execute given command. */
((ExecutableWithNamespace)runClass.getDeclaredConstructor().newInstance()).execute(ns, dockerfileGitHubUtil);
Expand All @@ -57,30 +65,30 @@
ArgumentParsers.newFor("dockerfile-image-update").addHelp(true).build()
.description("Image Updates through Pull Request Automator");

parser.addArgument("-l", "--" + Constants.GIT_API_SEARCH_LIMIT)
parser.addArgument("-l", "--" + GIT_API_SEARCH_LIMIT)
.type(Integer.class)
.setDefault(1000)
.help("limit the search results for github api (default: 1000)");
parser.addArgument("-o", "--" + Constants.GIT_ORG)
parser.addArgument("-o", "--" + GIT_ORG)
.help("search within specific organization (default: all of github)");
/* Currently, because of argument passing reasons, you can only specify one branch. */
parser.addArgument("-b", "--" + Constants.GIT_BRANCH)
parser.addArgument("-b", "--" + GIT_BRANCH)
.help("make pull requests for given branch name (default: main)");
parser.addArgument("-g", "--" + Constants.GIT_API)
parser.addArgument("-g", "--" + GIT_API)
.help("link to github api; overrides environment variable");
parser.addArgument("-f", "--auto-merge").action(Arguments.storeTrue())
.help("NOT IMPLEMENTED / set to automatically merge pull requests if available");
parser.addArgument("-m")
.help("message to provide for pull requests");
parser.addArgument("-c")
.help("additional commit message for the commits in pull requests");
parser.addArgument("-e", "--" + Constants.GIT_REPO_EXCLUDES)
parser.addArgument("-e", "--" + GIT_REPO_EXCLUDES)
.help("regex of repository names to exclude from pull request generation");
parser.addArgument("-B")
.help("additional body text to include in pull requests");
parser.addArgument("-s", "--" + Constants.SKIP_PR_CREATION)
parser.addArgument("-s", "--" + SKIP_PR_CREATION)
.type(Boolean.class)
.setDefault(false)
.setDefault(false) //To prevent null from being returned by the argument
.help("Only update image tag store. Skip creating PRs");
parser.addArgument("-x")
.help("comment snippet mentioned in line just before 'FROM' instruction(Dockerfile)" +
Expand All @@ -95,14 +103,19 @@
.type(String.class)
.required(false)
.help("Use RateLimiting when sending PRs. RateLimiting is enabled only if this value is set it's disabled by default.");
parser.addArgument("-d", "--" + DEBUG)
.type(Boolean.class)
.setDefault(false) //To prevent null from being returned by the argument
.required(false)
.help("Enable debug logging, including git wire logs.");
return parser;
}

/* Adding subcommands to the subcommands list.
argparse4j allows commands to be truncated, so users can type the first letter (a,c,p) for commands */
public static Set<ClassPath.ClassInfo> findSubcommands(ArgumentParser parser) throws IOException {
Subparsers subparsers = parser.addSubparsers()
.dest(Constants.COMMAND)
.dest(COMMAND)
.help("FEATURE")
.title("subcommands")
.description("Specify which feature to perform")
Expand Down Expand Up @@ -181,27 +194,65 @@
return runClass;
}

public static String gitApiToken() {
return gitApiToken(System::getenv, System::exit);

Check warning on line 198 in dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/CommandLine.java

View check run for this annotation

Codecov / codecov/patch

dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/CommandLine.java#L198

Added line #L198 was not covered by tests
}

public static String gitApiToken(final Function<String, String> envFunc, final Consumer<Integer> exitFunc) {
final String token = envFunc.apply("git_api_token");
if (Objects.isNull(token)) {
log.error("Please provide GitHub token in environment variables.");
exitFunc.accept(3);
}
return token;
}
Comment on lines +201 to +208
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved from within initializeDockerfileGithubUtil


public static String gitApiUrl(final Namespace ns) throws IOException {
return gitApiUrl(ns, System::getenv);

Check warning on line 211 in dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/CommandLine.java

View check run for this annotation

Codecov / codecov/patch

dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/CommandLine.java#L211

Added line #L211 was not covered by tests
}

public static String gitApiUrl(final Namespace ns, final Function<String, String> envFunc) throws IOException {
return Optional.ofNullable(
Optional.ofNullable(ns.getString(GIT_API))
.orElse(envFunc.apply("git_api_url"))
)
.orElseThrow(() -> new IOException("No Git API URL in environment variables nor on the commmand line."));
}
Comment on lines +214 to +220
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved from within initializeDockerfileGithubUtil


public static DockerfileGitHubUtil initializeDockerfileGithubUtil(
final String gitApiUrl,
final String token,
final boolean debug) throws IOException
{
return initializeDockerfileGithubUtil(gitApiUrl, token, () -> new GitHubBuilder(), debug);

Check warning on line 227 in dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/CommandLine.java

View check run for this annotation

Codecov / codecov/patch

dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/CommandLine.java#L227

Added line #L227 was not covered by tests
}

/* Validate API URL and connect to the API using credentials. */
public static DockerfileGitHubUtil initializeDockerfileGithubUtil(String gitApiUrl) throws IOException {
if (gitApiUrl == null) {
gitApiUrl = System.getenv("git_api_url");
if (gitApiUrl == null) {
throw new IOException("No Git API URL in environment variables.");
}
}
String token = System.getenv("git_api_token");
if (token == null) {
log.error("Please provide GitHub token in environment variables.");
System.exit(3);
}
public static DockerfileGitHubUtil initializeDockerfileGithubUtil(
final String gitApiUrl,
final String token,
final Supplier<GitHubBuilder> builderFunc,
final boolean debug) throws IOException {

GitHub github = new GitHubBuilder().withEndpoint(gitApiUrl)
GitHub github = shouldAddWireLogger(builderFunc.get(), debug)
.withEndpoint(gitApiUrl)
.withOAuthToken(token)
.build();
github.checkApiUrlValidity();

GitHubUtil gitHubUtil = new GitHubUtil(github);
return new DockerfileGitHubUtil(new GitHubUtil(github));
}

public static GitHubBuilder shouldAddWireLogger(final GitHubBuilder builder, final boolean debug) {
if (debug) {
HttpLoggingInterceptor logger = new HttpLoggingInterceptor();
logger.setLevel(HttpLoggingInterceptor.Level.HEADERS);
logger.redactHeader("Authorization");

return new DockerfileGitHubUtil(gitHubUtil);
builder.withConnector(new OkHttpGitHubConnector(new OkHttpClient.Builder()
.addInterceptor(logger)
.build()));
}
return builder;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public void execute(final Namespace ns, DockerfileGitHubUtil dockerfileGitHubUti
log.error("Could not initialize the Image tage store. Exception: ", e);
}

if (ns.get(Constants.SKIP_PR_CREATION)) {
if (ns.getBoolean(Constants.SKIP_PR_CREATION)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes error of trying to cast Object to boolean

log.info("Since the flag {} is set to True, the PR creation steps will "
+ "be skipped.", Constants.SKIP_PR_CREATION);
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ private Constants() {
public static final String IGNORE_IMAGE_STRING = "x";
public static final String FILE_NAMES_TO_SEARCH = "filenamestosearch";
public static final String RATE_LIMIT_PR_CREATION = "rate_limit_pr_creations";
public static final String DEBUG = "debug";
//max number of PRs to be sent (or tokens to be added) per DEFAULT_RATE_LIMIT_DURATION(per hour in this case)
public static final long DEFAULT_RATE_LIMIT = 60;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public DockerfileGitHubUtil(GitHubUtil gitHubUtil) {
this.gitHubUtil = gitHubUtil;
}

protected GitHubUtil getGitHubUtil() { return gitHubUtil; }
public GitHubUtil getGitHubUtil() { return gitHubUtil; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Visible for testing


/**
* Return an existing fork in the current user's org or create one if it does not exist
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,28 @@
package com.salesforce.dockerfileimageupdate;

import com.google.common.reflect.ClassPath;
import com.salesforce.dockerfileimageupdate.utils.DockerfileGitHubUtil;

import org.kohsuke.github.GitHub;
import org.kohsuke.github.GitHubBuilder;
import org.kohsuke.github.extras.okhttp3.OkHttpGitHubConnector;
import org.mockito.Mockito;
import org.testng.annotations.Test;

import net.sourceforge.argparse4j.inf.Namespace;

import java.io.IOException;
import java.util.Arrays;
import java.util.List;
import java.util.Set;
import java.util.function.Consumer;
import java.util.function.Function;

import static com.salesforce.dockerfileimageupdate.utils.Constants.GIT_API;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.never;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertSame;

/**
* Created by afalko on 10/25/17.
Expand All @@ -31,4 +45,91 @@ public void testLoadSubcommands() throws Exception {
classInfo -> assertThat(expectedSubCommands).contains(classInfo.getSimpleName())
);
}

@Test
public void testShouldAddWireLoggerDisabled() {
GitHubBuilder builder = Mockito.spy(new GitHubBuilder());
CommandLine.shouldAddWireLogger(builder, false);

Mockito.verify(builder, never()).withConnector(Mockito.any(OkHttpGitHubConnector.class));
}

@Test
public void testShouldAddWireLoggerEnabled() {
GitHubBuilder builder = Mockito.spy(new GitHubBuilder());
CommandLine.shouldAddWireLogger(builder, true);

Mockito.verify(builder).withConnector(Mockito.any(OkHttpGitHubConnector.class));
}

@Test
public void testInitializeDockerfileGithubUtil() throws IOException {
GitHubBuilder builder = Mockito.spy(new GitHubBuilder());
GitHub github = Mockito.mock(GitHub.class);

Mockito.doReturn(github).when(builder).build();

DockerfileGitHubUtil retval = CommandLine.initializeDockerfileGithubUtil("someUrl", "someToken", () -> builder, true);

Mockito.verify(builder).withEndpoint(Mockito.eq("someUrl"));
Mockito.verify(builder).withOAuthToken(Mockito.eq("someToken"));
Mockito.verify(builder).withConnector(Mockito.any(OkHttpGitHubConnector.class));
Mockito.verify(github).checkApiUrlValidity();
assertSame(retval.getGitHubUtil().getGithub(), github);
}

@Test
public void testGitApiUrlNonNullNamespace() throws IOException {
Namespace ns = Mockito.mock(Namespace.class);
Mockito.doReturn("someUrl").when(ns).getString(GIT_API);

String retval = CommandLine.gitApiUrl(ns, x -> null);
assertEquals(retval, "someUrl");
}

@Test
public void testGitApiUrlNullNamespace() throws IOException {
Namespace ns = Mockito.mock(Namespace.class);
Mockito.doReturn(null).when(ns).getString(GIT_API);

String retval = CommandLine.gitApiUrl(ns, x -> "anotherUrl");
assertEquals(retval, "anotherUrl");
}

@Test(expectedExceptions = { IOException.class } )
public void testGitApiUrlNamespaceAndEnvNull() throws IOException {
Namespace ns = Mockito.mock(Namespace.class);
Mockito.doReturn(null).when(ns).getString(GIT_API);

String retval = CommandLine.gitApiUrl(ns, x -> null);
assertEquals(retval, "anotherUrl");
}

@Test
public void gitApiTokenAvailable() {
Function<String, String> envFunc = x -> {
assertEquals(x, "git_api_token");
return "token";
};

Consumer<Integer> exitFunc = new Consumer<Integer>() {
@Override
public void accept(Integer t) {}
};

final String retval = CommandLine.gitApiToken(envFunc, exitFunc);
assertEquals(retval, "token");
}

@Test
public void gitApiTokenNull() {
Function<String, String> envFunc = x -> {
assertEquals(x, "git_api_token");
return null;
};

Consumer<Integer> exitFunc = x -> assertEquals(x, 3);

CommandLine.gitApiToken(envFunc, exitFunc);
}
}
Loading