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

Jump to bottom

update multiple Dockerfiles of the same repo #25

Merged
merged 8 commits into from
Jul 26, 2018
Merged

update multiple Dockerfiles of the same repo #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1b48508
update multiple Dockerfiles of the same repo
jdoshi1 Jul 18, 2018
20483c5
remove unwanted debug logs
jdoshi1 Jul 18, 2018
44f2832
use constants for accessing commandline namespace values
jdoshi1 Jul 18, 2018
fc75976
rename method and minor fixes
jdoshi1 Jul 18, 2018
2a83bf9
Merge branch 'master' into update-multiple-dockerfiles
jdoshi1 Jul 19, 2018
77eb466
Merge branch 'master' into update-multiple-dockerfiles
jdoshi1 Jul 24, 2018
6796aa3
Merge branch 'master' into update-multiple-dockerfiles
jdoshi1 Jul 26, 2018
156a64a
use positive condition
jdoshi1 Jul 26, 2018
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
2 changes: 1 addition & 1 deletion dockerfile-image-update/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
<dependency>
<groupId>org.kohsuke</groupId>
<artifactId>github-api</artifactId>
<version>1.92</version>
<version>1.93</version>
</dependency>
</dependencies>

Expand Down
2 changes: 1 addition & 1 deletion dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/CommandLine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
* Created by minho-park on 6/29/2016.
*/
public class CommandLine {
private final static Logger log = LoggerFactory.getLogger(CommandLine.class);
private static final Logger log = LoggerFactory.getLogger(CommandLine.class);

/* Should never actually be instantiated in code */
private CommandLine () { }
Expand Down
150 changes: 112 additions & 38 deletions ...image-update/src/main/java/com/salesforce/dockerfileimageupdate/subcommands/impl/All.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

package com.salesforce.dockerfileimageupdate.subcommands.impl;

import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.Multimap;
import com.google.gson.JsonElement;
import com.google.gson.JsonParseException;
import com.google.gson.JsonParser;
Expand All @@ -20,6 +22,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
Expand All @@ -29,7 +32,7 @@
@SubCommand(help="updates all repositories' Dockerfiles",
requiredParams = {Constants.STORE})
public class All implements ExecutableWithNamespace {
private final static Logger log = LoggerFactory.getLogger(All.class);
private static final Logger log = LoggerFactory.getLogger(All.class);

private DockerfileGitHubUtil dockerfileGitHubUtil;

Expand All @@ -38,17 +41,18 @@ public void execute(final Namespace ns, final DockerfileGitHubUtil dockerfileGit
loadDockerfileGithubUtil(dockerfileGitHubUtil);

Map<String, String> imageToTagMap = new HashMap<>();
Map<String, String> parentToImage = new HashMap<>();
Map<String, String> parentToPath = new HashMap<>();
Multimap<String, String> imagesFoundInParentRepo = ArrayListMultimap.create();
Multimap<String, String> pathToDockerfilesInParentRepo = ArrayListMultimap.create();

Set<Map.Entry<String, JsonElement>> imageToTagStore = parseStoreToImagesMap(ns.get(Constants.STORE));
for (Map.Entry<String, JsonElement> imageToTag : imageToTagStore) {
String image = imageToTag.getKey();
log.info("Repositories with image {} being forked.", image);
imageToTagMap.put(image, imageToTag.getValue().getAsString());
PagedSearchIterable<GHContent> contentsWithImage =
this.dockerfileGitHubUtil.findFilesWithImage(image, ns.get("o"));
forkRepositoriesFound(parentToPath, parentToImage, contentsWithImage, image);
this.dockerfileGitHubUtil.findFilesWithImage(image, ns.get(Constants.GIT_ORG));
forkRepositoriesFound(pathToDockerfilesInParentRepo,
imagesFoundInParentRepo, contentsWithImage, image);
}

GHMyself currentUser = this.dockerfileGitHubUtil.getMyself();
Expand All @@ -57,32 +61,43 @@ public void execute(final Namespace ns, final DockerfileGitHubUtil dockerfileGit
}

log.info("Retrieving all the forks...");
PagedIterable<GHRepository> repos = dockerfileGitHubUtil.getGHRepositories(parentToPath, currentUser);
PagedIterable<GHRepository> listOfCurrUserRepos =
dockerfileGitHubUtil.getGHRepositories(pathToDockerfilesInParentRepo, currentUser);

String message = ns.get("m");
List<IOException> exceptions = new ArrayList<>();
for (GHRepository repo : repos) {
List<String> skippedRepos = new ArrayList<>();

for (GHRepository currUserRepo : listOfCurrUserRepos) {
try {
changeDockerfiles(parentToPath, parentToImage, imageToTagMap, repo, message, ns.get("c"));
changeDockerfiles(ns, pathToDockerfilesInParentRepo, imagesFoundInParentRepo, imageToTagMap, currUserRepo,
skippedRepos);
} catch (IOException e) {
exceptions.add(e);
}
}

if (exceptions.size() > 0) {
if (!exceptions.isEmpty()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Positive conditions are generally more readable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep agreed but in this case, isn't isEmpty() more readable/preferred than size() > 0?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll leave it alone; not going to block things on nitpicks :)

log.info("There were {} errors with changing Dockerfiles.", exceptions.size());
throw exceptions.get(0);
}

if (!skippedRepos.isEmpty()) {
log.info("List of repos skipped: {}", skippedRepos);
}
}

protected void loadDockerfileGithubUtil(DockerfileGitHubUtil _dockerfileGitHubUtil) {
dockerfileGitHubUtil = _dockerfileGitHubUtil;
}

protected void forkRepositoriesFound(Map<String, String> parentToPath,
Map<String, String> parentToImage,
PagedSearchIterable<GHContent> contentsWithImage, String image) throws IOException {
protected void forkRepositoriesFound(Multimap<String, String> pathToDockerfilesInParentRepo,
Multimap<String, String> imagesFoundInParentRepo,
PagedSearchIterable<GHContent> contentsWithImage,
String image) throws IOException {
log.info("Forking {} repositories...", contentsWithImage.getTotalCount());
List<String> parentReposForked = new ArrayList<>();
GHRepository parent;
String parentRepoName = null;
for (GHContent c : contentsWithImage) {
/* Kohsuke's GitHub API library, when retrieving the forked repository, looks at the name of the parent to
* retrieve. The issue with that is: GitHub, when forking two or more repositories with the same name,
Expand All @@ -91,12 +106,25 @@ protected void forkRepositoriesFound(Map<String, String> parentToPath,
* repositories that were automatically fixed by GitHub. Instead, we save the names of the parent repos
* in the map above, find the list of repositories under the authorized user, and iterate through that list.
*/
GHRepository parent = c.getOwner();
log.info("Forking {}...", parent.getFullName());
parentToPath.put(c.getOwner().getFullName(), c.getPath());
parentToImage.put(c.getOwner().getFullName(), image);
dockerfileGitHubUtil.checkFromParentAndFork(parent);
parent = c.getOwner();
parentRepoName = parent.getFullName();
if (parent.isFork()) {
log.warn("Skipping {} because it's a fork already. Sending a PR to a fork is unsupported at the moment.",
parentRepoName);
} else {
pathToDockerfilesInParentRepo.put(parentRepoName, c.getPath());
imagesFoundInParentRepo.put(parentRepoName, image);

// fork the parent if not already forked
if (!parentReposForked.contains(parentRepoName)) {
dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(parent);
parentReposForked.add(parentRepoName);
}
}
}

log.info("Path to Dockerfiles in repo '{}': {}", parentRepoName, pathToDockerfilesInParentRepo);
log.info("All images found in repo '{}': {}", parentRepoName, imagesFoundInParentRepo);
}

protected Set<Map.Entry<String, JsonElement>> parseStoreToImagesMap(String storeName)
Expand All @@ -109,7 +137,7 @@ protected Set<Map.Entry<String, JsonElement>> parseStoreToImagesMap(String store
store.getDefaultBranch());

if (storeContent == null) {
return null;
return Collections.emptySet();
}

JsonElement json;
Expand All @@ -118,43 +146,89 @@ protected Set<Map.Entry<String, JsonElement>> parseStoreToImagesMap(String store
json = new JsonParser().parse(streamR);
} catch (JsonParseException e) {
log.warn("Not a JSON format store.");
return null;
return Collections.emptySet();
}
}

JsonElement imagesJson = json.getAsJsonObject().get("images");
return imagesJson.getAsJsonObject().entrySet();
}

protected void changeDockerfiles(Map<String, String> parentToPath,
Map<String, String> parentToImage,
protected void changeDockerfiles(Namespace ns,
Multimap<String, String> pathToDockerfilesInParentRepo,
Multimap<String, String> imagesFoundInParentRepo,
Map<String, String> imageToTagMap,
GHRepository initialRepo, String message, String commitMessage) throws IOException, InterruptedException {
GHRepository currUserRepo,
List<String> skippedRepos) throws IOException, InterruptedException {
/* The Github API does not provide the parent if retrieved through a list. If we want to access its parent,
* we need to retrieve it once again.
*/
GHRepository retrievedRepo;
if (initialRepo.isFork()) {
retrievedRepo = dockerfileGitHubUtil.getRepo(initialRepo.getFullName());
GHRepository forkedRepo;
if (currUserRepo.isFork()) {
try {
forkedRepo = dockerfileGitHubUtil.getRepo(currUserRepo.getFullName());
} catch (FileNotFoundException e) {
/* The edge case here: If a different command calls getGHRepositories, and then this command calls
* it again within 60 seconds, it will still have the same list of repositories (because of caching).
* However, between the previous and current call, if some of those repositories are deleted, the call
* above may cause a FileNotFoundException. This clause prevents that exception from stopping our call;
* we do not need to stop because getGHRepositories checks that we have all the repositories we need.
*
* The integration test calls the testParent -> testAllCommand -> testIdempotency, and the
* testIdempotency was failing because of this edge condition.
*/

log.warn("This repository does not exist. The list of repositories must be outdated, but the list" +
"contains the repositories we need, so we ignore this error.");
return;
}
} else {
return;
}
GHRepository parent = retrievedRepo.getParent();
GHRepository parent = forkedRepo.getParent();

if (parent == null || !parentToPath.containsKey(parent.getFullName())) {
if (parent == null || !pathToDockerfilesInParentRepo.containsKey(parent.getFullName())) {
return;
}
log.info("Fixing Dockerfiles in {}...", initialRepo.getFullName());
String parentName = parent.getFullName();
String image = parentToImage.get(parentName);
String tag = imageToTagMap.get(image);
String branch = retrievedRepo.getDefaultBranch();
GHContent content = dockerfileGitHubUtil.tryRetrievingContent(retrievedRepo, parentToPath.get(parentName),
branch);
dockerfileGitHubUtil.modifyOnGithub(content, branch, image, tag, commitMessage);
dockerfileGitHubUtil.createPullReq(parent, branch, retrievedRepo, message);
}

log.info("Fixing Dockerfiles in {}...", forkedRepo.getFullName());
String parentName = parent.getFullName();
String branch = (ns.get(Constants.GIT_BRANCH) == null) ? forkedRepo.getDefaultBranch() : ns.get(Constants.GIT_BRANCH);

String pathToDockerfile;
String image;
String tag;
GHContent content;
boolean isContentModified = false;
boolean isRepoSkipped = true;

Iterator<String> pathToDockerfileInParentRepoIterator = pathToDockerfilesInParentRepo.get(parentName).iterator();
Iterator<String> imagesFoundInParentRepoIterator = imagesFoundInParentRepo.get(parentName).iterator();

while (pathToDockerfileInParentRepoIterator.hasNext()) {
pathToDockerfile = pathToDockerfileInParentRepoIterator.next();
image = imagesFoundInParentRepoIterator.next();
tag = imageToTagMap.get(image);
log.info("pathToDockerfile: {} , image: {}, tag: {}", pathToDockerfile, image, tag);
content = dockerfileGitHubUtil.tryRetrievingContent(forkedRepo, pathToDockerfile, branch);
if (content == null) {
log.info("No Dockerfile found at path: '{}'", pathToDockerfile);
} else {
dockerfileGitHubUtil.modifyOnGithub(content, branch, image, tag,
ns.get(Constants.GIT_ADDITIONAL_COMMIT_MESSAGE));
isContentModified = true;
isRepoSkipped = false;
}
}

if (isRepoSkipped) {
log.info("Skipping repo '{}' because contents of it's fork could not be retrieved. Moving ahead...",
parentName);
skippedRepos.add(forkedRepo.getFullName());
}

if (isContentModified) {
dockerfileGitHubUtil.createPullReq(parent, branch, forkedRepo, ns.get(Constants.GIT_PR_TITLE));
}
}
}
12 changes: 4 additions & 8 deletions ...age-update/src/main/java/com/salesforce/dockerfileimageupdate/subcommands/impl/Child.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@
@SubCommand(help="updates one specific repository with given tag",
requiredParams = {Constants.GIT_REPO, Constants.IMG, Constants.FORCE_TAG}, optionalParams = {"s", Constants.STORE})
public class Child implements ExecutableWithNamespace {
private final static Logger log = LoggerFactory.getLogger(Child.class);
private static final Logger log = LoggerFactory.getLogger(Child.class);

@Override
public void execute(final Namespace ns, final DockerfileGitHubUtil dockerfileGitHubUtil)
throws IOException, InterruptedException {
String branch = ns.get("b");
String branch = ns.get(Constants.GIT_BRANCH);
String img = ns.get(Constants.IMG);
String forceTag = ns.get(Constants.FORCE_TAG);

Expand All @@ -36,18 +36,14 @@ public void execute(final Namespace ns, final DockerfileGitHubUtil dockerfileGit

log.info("Retrieving repository and creating fork...");
GHRepository repo = dockerfileGitHubUtil.getRepo(ns.get(Constants.GIT_REPO));
GHRepository fork = dockerfileGitHubUtil.checkFromParentAndFork(repo);
GHRepository fork = dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(repo);

if (branch == null) {
branch = repo.getDefaultBranch();
}
log.info("Modifying on Github...");
dockerfileGitHubUtil.modifyAllOnGithub(fork, branch, img, forceTag);

String message = ns.get("m");


dockerfileGitHubUtil.createPullReq(repo, branch, fork, message);
dockerfileGitHubUtil.createPullReq(repo, branch, fork, ns.get(Constants.GIT_PR_TITLE));

/* TODO: A potential problem that requires a design decision:
* 1. Leave forks in authenticated repository.
Expand Down
Loading