-
Notifications
You must be signed in to change notification settings - Fork 10
AGDIGGER-59 trigger jenkins job - 2 #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AGDIGGER-59 trigger jenkins job - 2 #3
Conversation
ping @wtrocki @matzew @josemigallas |
.build(); | ||
} | ||
|
||
public static DiggerClientBuilder builder() { |
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.
IMO there's no point on having this method since DiggerClientBuilder is already public.
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.
Well, right..
I personally think that DiggerClient.builder().foo().bar().build()
is more fluent than new DiggerClientBuilder().foo().bar().build()
.
I am inspired by Guava in this idea.
I prefer keeping it for now, unless more objections arise.
|
||
Build a customized client: | ||
``` | ||
DiggerClient client = DiggerClient.builder() |
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.
Awesome idea!
} | ||
|
||
public static class DiggerClientBuilder { | ||
private JenkinsAuth auth; |
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.
Like it. That would be easy to test.
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.
Reviewed and verified. Good to merge
Follow up of #2