Downstream test and upgrade unified #352
Conversation
| @@ -15,9 +15,9 @@ | |||
| /** | |||
| * This task run external process and additionally store output of external process to file. | |||
| */ | |||
| public class RunTestReleaseTask extends DefaultTask { | |||
| public class RunDownstreamTestTask extends DefaultTask { | |||
There was a problem hiding this comment.
It seems that the name of this task is too specific. The task only runs an arbitrary command, and is not coupled with downstream testing. The only thing that couples the implementation of this task with downstream testing is the repo name and log message emitted before the task runs, which can be managed differently, e.g.
How about calling this task 'SilentExecTask'?
|
|
||
| private static final Logger LOG = Logging.getLogger(RunTestReleaseTask.class); | ||
| private static final Logger LOG = Logging.getLogger(RunDownstreamTestTask.class); | ||
|
|
There was a problem hiding this comment.
The 'repoName' property on this task is somewhat awkward. It's only used for logging the message. Perhaps we can push the message to doFirst action? Or change the 'repoName' property to 'displayName'. Then, in code we would print
LOG.lifecycle(" Running {}. The output will be saved in {}", ...); | @@ -1,3 +1,3 @@ | |||
| rootProject.name="shipkit" | |||
|
|
|||
| include "downstreamTest" | |||
| include "testDownstream" | |||
There was a problem hiding this comment.
I like the new name! Consistent with our conventions!!!
| import java.util.regex.Matcher; | ||
| import java.util.regex.Pattern; | ||
|
|
||
| public class RepositoryNameUtil { |
There was a problem hiding this comment.
Nice utility class that encapsulates complexity in a single spot, making the codebase easy to read. Cool :)
| @@ -10,7 +10,8 @@ | |||
| import java.util.List; | |||
|
|
|||
| import static java.util.Arrays.asList; | |||
| import static org.shipkit.internal.gradle.util.StringUtil.capitalize; | |||
| import static org.apache.commons.lang.StringUtils.capitalize; | |||
There was a problem hiding this comment.
Is this change on purpose?
I'd rather stay with our StringUtil in order to minimize dependencies on external libs.
There was a problem hiding this comment.
Good catch, thanks! Definitely not on purpose :)
|
Suggestions applied and merging |
No description provided.