-
Notifications
You must be signed in to change notification settings - Fork 16
Conversation
...n/java/com/org/jenkins/custom/jenkins/distribution/service/services/UpdateCenterService.java
Show resolved
Hide resolved
src/main/java/com/org/jenkins/custom/jenkins/distribution/service/util/Util.java
Outdated
Show resolved
Hide resolved
public PluginController(UpdateCenterService updateCenterService) { | ||
this.updateCenterService = updateCenterService; | ||
} | ||
|
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.
Should I put its usage here ? like a sort of api docs
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.
Yes! Always include extra documentation, it's never bad to leave notes for people (and even yourself!) who will be looking at this later.
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.
There's a few things that could be updated around the try/catch. Essentially every time you have a try, catch!
public PluginController(UpdateCenterService updateCenterService) { | ||
this.updateCenterService = updateCenterService; | ||
} | ||
|
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.
Yes! Always include extra documentation, it's never bad to leave notes for people (and even yourself!) who will be looking at this later.
...n/java/com/org/jenkins/custom/jenkins/distribution/service/services/UpdateCenterService.java
Outdated
Show resolved
Hide resolved
public ResponseEntity<?> getPlugins() { | ||
LOGGER.info("Request Received"); | ||
try { | ||
return new ResponseEntity<>(updateCenterService.downloadUpdateCenterJSON().toMap(), HttpStatus.OK); |
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.
Does this generic have a type?
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.
It is returning a Hash map, but I read it is best practise to leave it generic, if you want I could change it :)
This PR adds the controllers for pulling in the update center.json.