AGDIGGER-60 - allow to stream artifacts by wtrocki · Pull Request #1 · aliok/digger-java

wtrocki · 2016-12-09T20:34:21Z

Motivation

Allow to stream jenkins artifacts by name.

…nkins-job AGDIGGER-59 trigger jenkins job

aliok · 2016-12-12T09:34:52Z

pom.xml

+      <dependency>
+        <groupId>org.mockito</groupId>
+        <artifactId>mockito-core</artifactId>
+        <version>${mockito-core.version}</version>


Good point: let's use properties for versions... I will do this for the rest of the deps

@wtrocki as we talked. maybe you can do it for the rest of the deps too.

IMO it's a bit redundant nevertheless since they are used only in one place each

but it's usually much nicer to have those covered in a properties section

aliok · 2016-12-12T09:36:11Z

src/test/java/digkins/services/ArtifactsServiceTests.java

+
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.mockito.BDDMockito.given;


@wtrocki I had the old TDD style used in other tests. I think we should use one approach.
Unless you have strong opinions about BDD, I will convert given to when

Ok, I won't do it. you do it :)

+1 on when, same as in UPS

Sorry. We did not had any tests when I started working on that. Fully agree.

aliok · 2016-12-12T09:39:20Z

src/main/java/com/redhat/digkins/services/ArtifactsService.java

+
+      }
+    } catch (Exception e) {
+      LOG.error("Problem when fetching artifacts for {0} {1} {2}", jobName, buildNumber, artifactName, e);


I think it might be better to throw an exception instead of just returning null in case of an error.
Any objections?

matzew · 2016-12-12T09:44:48Z

src/test/java/digkins/services/ArtifactsServiceTests.java

+    given(job.getBuildByNumber(anyInt())).willReturn(build);
+
+    InputStream artifact = artifactsService.fetchArtifact("artifact", 1,"test");
+    assertNull(artifact);


assertj is much nicer

@wtrocki I've already introduced it. (this PR needs a rebase)

aliok · 2016-12-12T09:44:59Z

@wtrocki maybe add a how to in README file?

…nkins-job-2 AGDIGGER-59 trigger jenkins job - 2

josemigallas · 2016-12-12T09:46:14Z

pom.xml

+      <dependency>
+        <groupId>org.mockito</groupId>
+        <artifactId>mockito-core</artifactId>
+        <version>${mockito-core.version}</version>


IMO it's a bit redundant nevertheless since they are used only in one place each

josemigallas · 2016-12-12T09:49:05Z

src/main/java/com/redhat/digkins/DiggerClient.java

+   */
+  public InputStream fetchArtifact(String jobName, int buildNumber, String artifactName) {
+    ArtifactsService artifactsService = new ArtifactsService(jenkins);
+    return artifactsService.fetchArtifact(jobName,buildNumber,artifactName);


There are some spaces after commas missing here

josemigallas · 2016-12-12T09:49:52Z

src/main/java/com/redhat/digkins/services/ArtifactsService.java

+ */
+public class ArtifactsService {
+
+  private JenkinsServer jenkins;


Should this be final?

good call, I also miss a bit of final usage on various places here

It cannot be final now after rebase and api change.

josemigallas · 2016-12-12T09:57:03Z

src/main/java/com/redhat/digkins/services/ArtifactsService.java

+    try {
+      JobWithDetails job = jenkins.getJob(jobName);
+      Build build = job.getBuildByNumber(buildNumber);
+      if (build instanceof BuildWithDetails) {


IMO this function body is a bit confusing. I would add an else block instead of having multiple return statements. When return statements are close it's easy to understand it but here there are a try-catch block, an if-else block and two return all mixed up.

A quick solution I would made is to inverse the predicate:

if (!(build instanceof BuildWithDetails)) { return ... ; // or better -> throw new WhatEverException(); }

josemigallas · 2016-12-12T09:58:45Z

src/main/java/com/redhat/digkins/services/ArtifactsService.java

+        }
+
+      }
+    } catch (Exception e) {


Couldn't this be a bit more specific? So we can identify the kind of Exception we are actually expecting.

+1000 fine grain is much better - Exception is almost all, execpt for Error :)

josemigallas · 2016-12-12T10:01:35Z

src/test/java/digkins/services/ArtifactsServiceTests.java

+
+public class ArtifactsServiceTests {
+  private String jobLocation = "http://localhost/job/artifact/";
+  private JenkinsServer server = mock(JenkinsServer.class);


I am more fond of @mock annotation

josemigallas · 2016-12-12T10:02:33Z

src/test/java/digkins/services/ArtifactsServiceTests.java

+import static org.mockito.Mockito.verify;
+
+public class ArtifactsServiceTests {
+  private String jobLocation = "http://localhost/job/artifact/";


I would avoid field initialisation, instead I find more convenient doing it inside @before method

josemigallas · 2016-12-12T10:05:36Z

src/test/java/digkins/services/ArtifactsServiceTests.java

+  private JenkinsServer server = mock(JenkinsServer.class);
+
+  @Test
+  public void testGetNoArtifacts() throws Exception {


IMHO tests naming should be more explicit, it's hard to understand what this test is about for example

aliok · 2017-05-30T09:59:08Z

@wtrocki FYI

wtrocki force-pushed the AGDIGGER-57 branch from e8e4ff4 to 1ca48b6 Compare December 10, 2016 10:24

wtrocki mentioned this pull request Dec 11, 2016

AGDIGGER-59 trigger jenkins job aerogear-attic/aerogear-digger-java-client#2

Merged

aliok added 2 commits December 12, 2016 12:18

Make services stateless and delays parametrized

2271ff0

Merge pull request aerogear-attic#2 from aliok/AGDIGGER-59-trigger-je…

795eb04

…nkins-job AGDIGGER-59 trigger jenkins job

aliok reviewed Dec 12, 2016

View reviewed changes

matzew reviewed Dec 12, 2016

View reviewed changes

Merge pull request aerogear-attic#3 from aliok/AGDIGGER-59-trigger-je…

641cd1b

…nkins-job-2 AGDIGGER-59 trigger jenkins job - 2

josemigallas reviewed Dec 12, 2016

View reviewed changes

AGDIGGER-60 - allow to stream artifacts

7d6fb33

wtrocki force-pushed the AGDIGGER-57 branch from 1ca48b6 to d7a4ed2 Compare December 12, 2016 16:29

wtrocki mentioned this pull request Dec 12, 2016

AGDIGGER-60 - fetching and saving artifacts api aerogear-attic/aerogear-digger-java-client#4

Merged

AGDIGGER-60 - Fetching artifacts

5dc7963

wtrocki force-pushed the AGDIGGER-57 branch from d7a4ed2 to 5dc7963 Compare December 13, 2016 11:11

aliok closed this May 30, 2017

wtrocki mentioned this pull request May 30, 2017

Agdigger 57 aerogear-attic/aerogear-digger-java-client#15

Closed

wtrocki deleted the AGDIGGER-57 branch June 1, 2017 09:28

Conversation

wtrocki commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliok commented Dec 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliok commented May 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wtrocki commented Dec 9, 2016 •

edited

Loading