feat(runners): additions for runners api #226
Conversation
jgoldhammer
commented
Aug 4, 2018
jgoldhammer
commented
- add register and delete runner
- enhance runner entity for ipAdress field
- introduce first runners tests
- add register and delete runner - enhance runner entity for ipAdress field - introduce first runners tests
gmessner
left a comment
gmessner
left a comment
There was a problem hiding this comment.
The PR looks great, just one small change requested concerning the use of Preconditions.checkNotNull()
| Boolean runUntagged, Boolean locked, Integer maximumTimeout) throws GitLabApiException { | ||
|
|
||
| Preconditions.checkNotNull(token, "token is necessary for registering a new runner"); | ||
|
|
There was a problem hiding this comment.
No need to use Preconditions.checkNotNull() here, .withParam("token", token, true) will throw a exception if token is null or empty.
There was a problem hiding this comment.
I have made the changes, although I am a fan of fast fail code. Clear preconditions make it easier in my eyes for api users.
There was a problem hiding this comment.
Thanks for making the change.
Preconditions.checkNotNull() and GitLabApiForm.withParam() will produce almost identical results. With a well-worded exception message telling what parameter name was null or empty. Also, there are literally 1000's of other places this is done, if we wanted to use Preconditions, to be consistent we should apply it everywhere.
Additionally, Preconditions.checkNotNull() does not look for an empty string, GitLabApiForm.withParam() does.
| import org.gitlab4j.api.models.JobStatus; | ||
| import org.gitlab4j.api.models.Runner; | ||
| import org.gitlab4j.api.models.RunnerDetail; | ||
| import org.glassfish.jersey.internal.guava.Preconditions; |
There was a problem hiding this comment.
Do not need to import this if you make the suggested change in registerRunner() below
|
@jgoldhammer |
|
@jgoldhammer |
2 participants
