Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-6167] Support JNLP slave connection via HTTP proxy #27

Merged
merged 1 commit into from
Mar 20, 2015
Merged

[JENKINS-6167] Support JNLP slave connection via HTTP proxy #27

merged 1 commit into from
Mar 20, 2015

Conversation

shirosaki
Copy link
Contributor

We get proxy setting from http_proxy environment variable.
https://issues.jenkins-ci.org/browse/JENKINS-6167

@jenkinsadmin
Copy link
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@oleg-nenashev oleg-nenashev changed the title Support JNLP slave connection via HTTP proxy [JENKINS-6167] Support JNLP slave connection via HTTP proxy Oct 18, 2014
@oleg-nenashev
Copy link
Member

Please don't forget to reference JIRA issues from pull requests.
https://issues.jenkins-ci.org/browse/JENKINS-6167

oleg-nenashev
oleg-nenashev reviewed Oct 18, 2014
View reviewed changes
src/main/java/hudson/remoting/Engine.java
@@ -182,8 +186,23 @@ public void run() {
if(!s.endsWith("/")) s+='/';
URL salURL = new URL(s+"tcpSlaveAgentListener/");

String httpProxy = System.getenv("http_proxy");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also makes sense to allow the definition of this parameter from the command line (e.g. by defining system properties)

@oleg-nenashev
Copy link
Member

I know the patch is very old, hence it is not a good time to review it.
BTW, it makes sense to somehow refactor the code in order to remove code duplications and to introduce system properties support.

Probably, we could merge it to a branch and refactor in order to avoid extra efforts from @shirosaki

@daniel-beck
Copy link
Member

The usual way to have Java applications use proxies would be -Dhttp.proxyHost/-Dhttp.proxyPort as described here.

PR appears to be missing support for proxies requiring authentication.

@shirosaki shirosaki force-pushed the proxy branch 4 times, most recently from 123a4e6 to 6177dd1 Compare October 20, 2014 02:31
@shirosaki
Copy link
Contributor Author

Update the patch.

  • add -Dhttp.proxyHost / -Dhttp.proxyPort support
  • refactor to reduce duplicated code
  • add reference to JIRA in commit log

I don't know how to do with proxy authentication.

@shirosaki
[JENKINS-6167] Support JNLP slave connection via HTTP proxy
8c875cb 
We get proxy setting from http.proxyHost property or http_proxy
environment variable.
@kohsuke kohsuke merged commit 8c875cb into jenkinsci:master Mar 20, 2015
kohsuke added a commit that referenced this pull request Mar 20, 2015
@kohsuke
Merge pull request #27
08b910c 
Conflicts:
	src/main/java/hudson/remoting/Engine.java
@tangicolin
Copy link

What appends if we want to have http_proxy set to be able to do some step action using the proxy but we want to have a direct connexion between master and slave.
If a correctly understand this patch, the value of no_proxy is currently not take.

@etiennebec etiennebec mentioned this pull request Jun 1, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@shirosaki @jenkinsadmin @oleg-nenashev @daniel-beck @tangicolin @kohsuke