Skip to content

Commit

Permalink
Allow the default port to be more easily configured.
Browse files Browse the repository at this point in the history 
Docker images and other automation use cases often want to make Jenkins
use specific port. Today they do this via init.groovy.d with a
problematic sleep timeout, so allow them to control this value via
system property.
  • Loading branch information
@kohsuke
kohsuke committed Dec 19, 2015
1 parent 9a54623 commit 653fbdb
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion core/src/main/java/jenkins/model/Jenkins.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ protected void onModified() throws IOException {
* TCP slave agent port.
* 0 for random, -1 to disable.
*/
private int slaveAgentPort =0;
private int slaveAgentPort = Integer.getInteger(Jenkins.class.getName()+".slaveAgentPort",0);

/**
* Whitespace-separated labels assigned to the master as a {@link Node}.
Expand Down

12 comments on commit 653fbdb

@KostyaSha
Copy link
Member

Choose a reason for hiding this comment

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

@kohsuke could you do your changes using PRs?

@KostyaSha
Copy link
Member

Choose a reason for hiding this comment

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

👎 for such change and 👎 for direct commits into master even if you are @kohsuke .
This change doesn't follow #1914

@kohsuke
Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I do use PR when I think a change warrants that (for example this), and in other cases I push through J-on-J. This felt like such a trivial enough change that the latter was well justified. I also thought this is non-controversial, though apparently I was wrong there.

I'm not following the last part of your comment. Clearly I cannot depend on a feature that's not in the code yet.

@KostyaSha
Copy link
Member

Choose a reason for hiding this comment

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

This felt like such a trivial enough change that the latter was well justified.

Not only you depend on some changes in core. That's your point of view and core has many contributors that may think other. During review you may get other points of views.

Sorry, what part is not clear? One person from community spent time and picked all magic system variables and proposed change, after your commit it introduces other way of configuration again.

I checked and found that:

Your simple change (because of workflow?) leads to undocumented variables. Now other people should spend their time (rebasing proposed PR to pick your variable, update documentation) even on such trivial change.

@KostyaSha
Copy link
Member

Choose a reason for hiding this comment

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

Now i got this issue, why answer in header available after some delay at all? (CliPort connection fails, CLI switches to buggy http connection). Will be glad to see filled jira issue as it smells like a bug.

@KostyaSha
Copy link
Member

Choose a reason for hiding this comment

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

Or it not used for CLI?

@daniel-beck
Copy link
Member

Choose a reason for hiding this comment

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

Naming inconsistent with the corresponding option hudson.TcpSlaveAgentListener.hostName.

@daniel-beck
Copy link
Member

Choose a reason for hiding this comment

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

@kohsuke Does this capture the intended behavior correctly?

Specifies the default TCP slave agent port unless/until configured differently on the UI. -1 to disable, 0 for random port, other values for fixed port.

@KostyaSha
Copy link
Member

Choose a reason for hiding this comment

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

No replies, could be reverted?

@alexellis
Copy link

Choose a reason for hiding this comment

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

Can the TCP port also be configured through an environmental variable for use on Docker?

@kohsuke
Copy link
Member Author

@kohsuke kohsuke commented on 653fbdb May 3, 2016

Choose a reason for hiding this comment

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

@daniel-beck yes it does capture the intended behaviour correctly.

@alexellis When I did this change, there was an in-flight PR that enables such dual support for every similar configuration option, and I was assuming that when that comes in, we'll change this as well. I'm not seeing it in the core today and I cannot find that PR right now.

@daniel-beck
Copy link
Member

Choose a reason for hiding this comment

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

@kohsuke It's still open: #1914

Please sign in to comment.