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
Feature/slave tunnel #840
Feature/slave tunnel #840
Conversation
Should also close #348 |
@@ -27,6 +27,11 @@ | |||
# [*executors*] | |||
# Number of executors for this slave. (How many jenkins jobs can run simultaneously on this host.) | |||
# | |||
# [*tunnel*] |
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.
This looks great! If we used tunnel_host
and tunnel_port
it might be easier to validate the inputs and then we could compose $tunnel
inside the class.
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.
We could, however then we would also need to verify if one of those have been given or not to see if you have to print something because
"HOSTNAME:"
":PORT"
"HOSTNAME:PORT"
are all valid, while ":" is not
(source https://github.com/jenkinsci/jenkins/blob/master/war/src/main/webapp/help/system-config/master-slave/jnlp-tunnel.html)
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.
Gotcha, that part wasn't clear to me reading the doc string. Thanks!
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.
We could catch this using the data types. (regex magic maybe)
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 this @vStone?
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.
<3
bb7e7c0
to
5bd0c6c
Compare
@@ -27,6 +27,11 @@ | |||
# [*executors*] | |||
# Number of executors for this slave. (How many jenkins jobs can run simultaneously on this host.) | |||
# | |||
# [*tunnel*] |
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.
<3
Created jhooyberghs#1 to add some spec tests. You can see the diff at jhooyberghs@93ac390 It just adds tests for |
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.
LGTM.
The acceptance test failure should be "fixed" (marked as pending) on master. |
Added tests for existing open PR "Add tunnel flag for slaves" #796