-
Notifications
You must be signed in to change notification settings - Fork 16
Conversation
@kwhetstone https://ci.jenkins.io/blue/organizations/jenkins/Tools%2Fcustom-distribution-service/detail/PR-58/2/pipeline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Sladyn, I am done my review. I do not know react very much, so I focused on the Java instead.
src/main/java/com/org/jenkins/custom/jenkins/distribution/service/PackagerController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/org/jenkins/custom/jenkins/distribution/service/util/Util.java
Outdated
Show resolved
Hide resolved
src/main/java/com/org/jenkins/custom/jenkins/distribution/service/util/Util.java
Outdated
Show resolved
Hide resolved
src/main/java/com/org/jenkins/custom/jenkins/distribution/service/PackagerController.java
Outdated
Show resolved
Hide resolved
@sladyn98 When that's the failing step, it's better to go back to the previous phase to see if there was a masked hidden failure. In this job, it's failing to even run the maven commands since it can't find the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments.
src/main/java/com/org/jenkins/custom/jenkins/distribution/service/PackagerController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/org/jenkins/custom/jenkins/distribution/service/PackagerController.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An easy small fix, but important nevertheless. Good names are important because they enhance discoverability and search results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this PR has the Util.java, the other PR (#68) could use it. So I suggest getting this one in first.
...va/com/org/jenkins/custom/jenkins/distribution/service/services/PackagerDownloadService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/org/jenkins/custom/jenkins/distribution/service/PackagerController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/org/jenkins/custom/jenkins/distribution/service/PackagerController.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that adding comments and potentially figuring out what's going on in the plugins in the pom are going to be the most important pieces of getting this merged. After those changes are added, I'll change to approve.
src/main/java/com/org/jenkins/custom/jenkins/distribution/service/PackagerController.java
Show resolved
Hide resolved
src/main/java/com/org/jenkins/custom/jenkins/distribution/service/util/Util.java
Outdated
Show resolved
Hide resolved
Looks good to me! I have no further comments on this PR. Thanks @sladyn98 |
This PR adds the packager-config.yml download feature, and also fixes the nav-links.