-
-
Notifications
You must be signed in to change notification settings - Fork 303
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Make the path that jenkins is accessible on configureable #539
Make the path that jenkins is accessible on configureable #539
Conversation
via --httpPath Defaults to /
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.
Probably YAGNI but 🤷♂️ @oleg-nenashev requested it.
...p/src/main/java/io/jenkins/jenkinsfile/runner/bootstrap/commands/JenkinsLauncherOptions.java
Outdated
Show resolved
Hide resolved
@@ -77,7 +77,8 @@ protected ServletContext createWebServer() throws Exception { | |||
? new Server(launcherOptions.httpPort) | |||
: new Server(queuedThreadPool); | |||
|
|||
WebAppContext context = new WebAppContext(launcherOptions.warDir.getPath(), null); | |||
contextPath = launcherOptions.httpPath; |
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.
You may need to handle mistakes here:
- trailing
/
- empty string
- just
/
, whichWebAppContext
will like butgetURL
will not
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.
what do you want me to do with all of those. Just throw an Illegal Argument Exception? I agree that probably YAGNI so I don't want to waste too much time with error cases for something that'll probably not get used
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.
Just throw an Illegal Argument Exception?
Sure, just to make things more transparent.
I don't want to waste too much time with error cases for something that'll probably not get used
Agreed.
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.
done. Case 1 and 3 are covered by the same "ends with /" statement
…p/commands/JenkinsLauncherOptions.java Co-authored-by: Jesse Glick <jglick@cloudbees.com>
...p/src/main/java/io/jenkins/jenkinsfile/runner/bootstrap/commands/JenkinsLauncherOptions.java
Show resolved
Hide resolved
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.
Documentation would be nice
...p/src/main/java/io/jenkins/jenkinsfile/runner/bootstrap/commands/JenkinsLauncherOptions.java
Show resolved
Hide resolved
private String validateHttpPath(String httpPath) throws IllegalArgumentException { | ||
if (StringUtils.endsWith(httpPath, "/")) { | ||
throw new IllegalArgumentException("httpPath must not end with a trailing /"); | ||
} else if ("".equals(httpPath)) { |
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.
} else if ("".equals(httpPath)) { | |
} else if (StringUtils.isBlank(httpPath)) { |
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.
Not in this case. null
is a valid input (what you get when the user does not set --httpPath
) (and StringUtils.isBlank(null)
returns true)
@oleg-nenashev - documentation present now. The build failure seems to be infra-related so can you retrigger a build (or just merge if you are happy as the last commit was only to the readme) |
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.
Sorry for the review delay. Looks good to me!
@oleg-nenashev reminder 1.0-beta-29...master |
via
--httpPath
Defaults to
/
Follow on from #537 allowing for configuration