Improve context in diagnostic logs #73
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
justinharringa
commented
Jun 8, 2019
| @@ -72,13 +72,13 @@ public void execute(final Namespace ns, final DockerfileGitHubUtil dockerfileGit | |||
| changeDockerfiles(ns, pathToDockerfilesInParentRepo, imagesFoundInParentRepo, imageToTagMap, currUserRepo, | |||
| skippedRepos); | |||
| } catch (IOException e) { | |||
| log.error(String.format("Error changing Dockerfile for %s", currUserRepo.getName()), e); | |||
There was a problem hiding this comment.
Seems advantageous to go ahead and log this immediately to be able to envision flow a bit.
justinharringa
commented
Jun 8, 2019
| exceptions.add(e); | ||
| } | ||
| } | ||
|
|
||
| if (!exceptions.isEmpty()) { | ||
| log.info("There were {} errors with changing Dockerfiles.", exceptions.size()); | ||
| throw exceptions.get(0); | ||
| throw new IOException(String.format("There were %s errors with changing Dockerfiles.", exceptions.size())); |
There was a problem hiding this comment.
I'm not stuck on this approach but it seemed odd to log the message and then just throw the first exception.
justinharringa
commented
Jun 8, 2019
| @@ -142,7 +142,7 @@ protected void loadDockerfileGithubUtil(DockerfileGitHubUtil _dockerfileGitHubUt | |||
| } | |||
| } | |||
|
|
|||
| log.info("Path to Dockerfiles in repo '{}': {}", parentRepoName, pathToDockerfilesInParentRepo); | |||
| log.info("Path to Dockerfiles in repos: {}", pathToDockerfilesInParentRepo); | |||
There was a problem hiding this comment.
parentRepoName was the last repo (which doesn't really make sense when you're printing something for all of the repos.
justinharringa
commented
Jun 8, 2019
| @@ -167,7 +167,7 @@ public GHContent tryRetrievingContent(GHRepository repo, String path, String bra | |||
| boolean listOfReposHasRecentForks = true; | |||
| for (String s : pathToDockerfileInParentRepo.keySet()) { | |||
| String forkName = s.substring(s.lastIndexOf('/') + 1); | |||
| log.info(forkName); | |||
| log.info(String.format("Verifying that %s has been forked", forkName)); | |||
justinharringa
force-pushed
the
improve-diagnostic-logs
branch
from
b0aff97 to
21a902c
Compare
justinharringa
force-pushed
the
improve-diagnostic-logs
branch
from
21a902c to
0867228
Compare
|
@afalko any objections? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Will add context for each change. Overall, there were some confusing strings and it seems useful to log some of these things near their locus of error.