Proxysettings inside project configuration #95
Conversation
src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/stash/StashApiClient.java
Outdated
Show resolved
Hide resolved
|
Thanks for the pull request. Could you please describe your use case? I can understand why one would want different proxy settings than the ones used for UC, but defining this per project seems like an overkill to me. I think proxy settings is something that would be best handled on the agent (slave) level, not on project level. |
|
On the technical level, please cover the parts you are changing with unit tests. You can check recent PRs by @proski for examples |
| // } | ||
| // } | ||
| // } | ||
| if (Jenkins.getInstanceOrNull() != null && proxy != null) { |
There was a problem hiding this comment.
What's the point of checking Jenkins.getInstanceOrNull() now that the code uses the proxy field?
| @@ -81,6 +83,7 @@ public StashBuildTrigger( | |||
| String projectPath, | |||
| String cron, | |||
| String stashHost, | |||
| JSONObject proxySettings, | |||
There was a problem hiding this comment.
Is the JSONObject type required here?
It is basically a Map<String,Object>
I would prefer to use proper, strongly typed DTOs here
| </f:radioBlock> | ||
| <f:radioBlock name="proxyChoice" value="custom" title="Custom" checked="${(instance.getProxySettings() != null) ? instance.isProxyType('custom') : false}"> | ||
| <f:entry title="Proxy Host" field="proxyHost"> | ||
| <f:textbox value="${(instance.getProxySettings() != null && instance.isProxyType('custom')) ? instance.getProxyDetails().getString('proxyHost') : ''}" /> |
There was a problem hiding this comment.
there is significant repetition here
I would expect jelly has some kind of a block/panel element that would enable you to hide irrelevant fields with one toggle
| @@ -125,6 +129,50 @@ public String getStashHost() { | |||
| return stashHost; | |||
| } | |||
|
|
|||
| public JSONObject getProxySettings() { | |||
There was a problem hiding this comment.
why are those getters needed? I think I only see getProxy() being actually called
|
Ideally, we should configure Bitbucket servers in the global configuration and select one of them in the project configuration. Speaking of the unit tests, it's a tangled mess. Stash Notifier plugin has much better code for interacting with Bitbucket. It uses the latest client code. It supports proxies. It also supports HTTPS certificates, which makes sense, as the code is one of the most valuable resources in the company and should be protected. The code also avoids having a separate |
I plan to try it eventually. Maybe we could use the unit tests code first. |
Added proxy (with credentials) settings for each project.
It can be chosen between inheritance from jenkins ( /pluginManager/advanced ) and custom proxy configuration.