Feature/GitHub support (1/3) #36
Conversation
*parse json to search for the correct state
*to test rebaseNeeded(PullRequest) *to test isApproved(PullRequest) *to test getAllPullRequests(Repository) *add json to test GithubService methods *make objects immutable
*to check if there are successful builds for the PullRequest
*add empty interface Provider
*make methods public to use interface *add methods to Provider *implement services *create class for polling *adapt polling to get config Objects
*to match yaml configuration
*edit methods to get the right RestTemplate
*adapt classes which called Services
*edit yml.example to realize Team as list
*Add ctor to Provider subclasses with parameter info from Provider methods *Create Provider subclasses with the specific RestTemplates to realize clarity *Adapt classes which called those objects
*to make delay value configurable *add default delay value *remove obsolete method
|
Simplest way is to review the resulting source code instead of the commits. |
|
|
||
| private File repoFolder( final RepositoryHost repoHost, final RepositoryTeam repoTeam, | ||
| final RepositoryConfig repoConfig ) { | ||
| return new File( new File( new File( workspace, repoHost.getUrl().getHost() ), repoTeam.getName() ), |
There was a problem hiding this comment.
You could use org.apache.commons.io.FileUtils#getFile to combine sub-paths
|
|
||
| private static String repoUrl( final RepositoryHost repoHost, final RepositoryTeam repoTeam, | ||
| final RepositoryConfig repoConfig ) { | ||
| return repoHost.getUrl() + "/" + repoTeam.getName() + "/" + repoConfig.getName() + ".git"; |
There was a problem hiding this comment.
Is the repo url setup always the same for every service? Otherwise it would be some task for the RepositoryConnector.
There was a problem hiding this comment.
For currently supported they are the same and also for GitLab, we can change this when needed some time in future.
| } | ||
|
|
||
| public static List<PullRequest> parsePullRequestsJson( final DocumentContext jsonPath ) { | ||
| final int numPullRequests = (int) jsonPath.read( "$.size" ); |
There was a problem hiding this comment.
Cast is not needed here, since read is generic.
| @Override | ||
| public PullRequest getLatestUpdate( final PullRequest pullRequest ) { | ||
| final DocumentContext jsonPath = jsonPathForPath( requestPath( pullRequest ) ); | ||
| final PullRequest updatedPullRequest = PullRequest.builder() // |
| final List<Integer> pullRequestAmount = jsonPath.read( "$..number" ); | ||
| final int numPullRequests = pullRequestAmount.size(); | ||
| final List<PullRequest> results = new ArrayList<>( numPullRequests ); | ||
| for ( int i = 0; i < numPullRequests; i++ ) { |
There was a problem hiding this comment.
You could use a IntStream.range if you like.
There was a problem hiding this comment.
No but I simplified the code there
| final List<String> states = jsonPath.read( "$..state" ); | ||
| boolean approved = false; | ||
| for ( final String state : states ) { | ||
| if ( state.equals( "APPROVED" ) ) { |
There was a problem hiding this comment.
Besides your lovely if ( a == true ) (simplify) 😛, you are effectively checking the last state in the list. I assume you want to check if all or any are approved?
Streams help here a lot.
There was a problem hiding this comment.
Jep nice code, I have overlooked this in refactoring ^^
| final List<String> commitIds = jsonPath.read( "$..sha" ); | ||
| final List<String> parentIds = jsonPath.read( "$..parents..sha" ); | ||
|
|
||
| return parentIds.stream().filter( parent -> commitIds.contains( parent ) ).findFirst() |
There was a problem hiding this comment.
We do not have any convention for streams yet, but I prefer each stream operation to be on a separate line for readability. There would be a few streams in here for adjustments.
There was a problem hiding this comment.
Should we use method references at filter?
|
|
||
| @Override | ||
| public void merge( final PullRequest pullRequest ) { | ||
| final String message = String.format( "Merged in %s (pull request #%d) by ReBaZer", pullRequest.getSource(), |
| public boolean greenBuildExists( final PullRequest pullRequest ) { | ||
| final String urlPath = "/commits/" + pullRequest.getSource() + "/status"; | ||
| final DocumentContext jsonPath = jsonPathForPath( urlPath ); | ||
| return jsonPath.<List<String>> read( "$.statuses[*].state" ).stream().anyMatch( s -> s.equals( "success" ) ); |
There was a problem hiding this comment.
You should use success".equals(s) to avoid NPE and then it can be a method reference.
There was a problem hiding this comment.
fixed, and some other similar cases
| @Override | ||
| public boolean greenBuildExists( final PullRequest pullRequest ) { | ||
| final DocumentContext jsonPath = jsonPathForPath( requestPath( pullRequest ) + "/statuses" ); | ||
| return jsonPath.<List<String>> read( "$.values[*].state" ).stream().anyMatch( s -> s.equals( "SUCCESSFUL" ) ); |
There was a problem hiding this comment.
Same as above: "SUCCESSFUL".equals(s)
|
Before merging, should we squash everything. Since 76 commits are quite many? |
No description provided.