enable configuration of an alternative Jenkins root URL for hooks #84
Conversation
src/main/java/io/jenkins/plugins/gitlabbranchsource/GitLabHookCreator.java
Outdated
Show resolved
Hide resolved
src/main/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServer.java
Outdated
Show resolved
Hide resolved
|
added a third commit to fix javadoc (d382aab) |
|
This is definitely a better approch than #57 |
| * @return a webhook or systemhook URL | ||
| */ | ||
| public static String getHookUrl(String hooksRootUrl, boolean isWebHook) { | ||
| String rootUrl = (hooksRootUrl == null) ? Jenkins.get().getRootUrl() : hooksRootUrl; |
There was a problem hiding this comment.
I might be blind, do we have tests for both cases here?
There was a problem hiding this comment.
do we have tests for both cases here?
Yes, in GitLabHookCreatorParameterizedTest, hookUrl() vs. hookUrlFromCustomRootUrl(). They both use the same set of test parameters, but:
- the former uses the URL param as a Jenkins root URL, with
hooksRootUrl=null(that's a test which was already there and which I've only slightly modified) - the later uses it as a
hooksRootUrl, with a dummy value for the Jenkins root URL (that's the test I've added)
|
@thomasgl-orange CI is failing on imports |
Right, thanks for the heads-up, fixed in f2f216b |
|
@thomasgl-orange Thanks for this fix. LGTM. @jetersen I was in favour of #57. But since we can add localhost to our /etc/hosts I think users had a workaround in that case. Here we are offered a little more flexibility, I think it is for good. Perhaps we should fill the field with Jenkins Root Url by default? |
| * Can be either a root URL with its trailing slash, or {@code null}. | ||
| */ | ||
| @CheckForNull | ||
| public String getHooksRootUrl() { |
There was a problem hiding this comment.
| public String getHooksRootUrl() { | |
| public String getHooksRootUrl() { | |
| if (hooksRootUrl == null || hooksRootUrl.isEmpty()) { | |
| return Jenkins.get().getRootUrl(); | |
| } |
There was a problem hiding this comment.
I think I prefer keeping the getter return value close enough to what's actually stored in the GitLabServer config object, and the "what default value to fall back to" logic in the GitLabHookCreator.getHookUrl(...) implementation.
Saying that made me think of doing 2e9b787 to simplify things a tiny bit.
There was a problem hiding this comment.
I feel the server configuration should also have a field like SCMTraits where user can add their configurations rather than a default empty field. Well, these changes might require a bit of extra remodelling so for now I think we are good.
There was a problem hiding this comment.
a default empty field is how both GitHub- and Bitbucket branch source plugin handles the custom hook url 👍
I had the pretty written the same words as @thomasgl-orange in my pending review to this comment 😆
There was a problem hiding this comment.
@thomasgl-orange Let me know when you are ready to merge this pr.
@baymac It's good to merge I think, I have nothing left to add/change. Thank you both for being so reactive on reviewing this PR! |
In my company, because of a "complicated" network architecture, the main root URLs of our Jenkins servers (
Jenkins#getRootUrl()) can not be reached from the Gitlab server. So the webhooks thatgitlab-branch-source-pluginwould create automatically can't work.This PR adds a new config option to
GitlabServer, an alternative root URL to use in the {system,web} hooks URLs generated by the plugin. It is of course optional,Jenkins#getRootUrl()is still used when this option is left blank.