-
Notifications
You must be signed in to change notification settings - Fork 831
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
update workspace url to allow full job names #109
Conversation
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
@@ -44,6 +44,7 @@ jenkinsPlugin { | |||
|
|||
dependencies { | |||
compile project(':job-dsl-core') | |||
optionalJenkinsPlugins([group: 'org.jenkins-ci.plugins', name: 'ant', version: '1.2', ext: 'jar']) |
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.
why did you add an optional dependency to the ant plugin?
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.
Without that, specs using JenkinsRule fail with java.lang.NoClassDefFoundError: hudson/tasks/Ant$AntInstallation
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.
Would it help if we update to the latest long-term support core? If not, can you create an issue in the Jenkins bug tracker for this and add a link to the issue in the source?
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 sure. I tried to update the core but couldn't get it to work (see https://groups.google.com/forum/#!topic/job-dsl-plugin/9li8Cqd7ENc)
I can add a ticket for this. Would it go towards the job-dsl plugin or the gradle-jpi plugin?
BTW I noticed a reference to the same issue here: https://github.com/jenkinsci/job-dsl-plugin/blob/master/job-dsl-plugin/src/test/groovy/javaposse/jobdsl/plugin/SeedJobTest.groovy#L13
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.
There is already a ticket for this: https://issues.jenkins-ci.org/browse/JENKINS-17129. Please add a reference so that we can remove the workaround after it got fixed.
I tried to build your branch, but I got test failures:
|
Strange. Works for me locally and on buildhive Is there any more detail for those failures? |
There seems to be an erroneous backslash. I'm using Windows...
|
Thanks. Tests should now pass in Windows. |
import hudson.model.AbstractProject | ||
import jenkins.model.Jenkins | ||
|
||
class WorkspaceProtocol { |
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.
Why do we need another class for this? This introduces a cyclic class dependency: WorkspaceProtocol > WorkspaceUrlHandler > WorkspaceUrlConnection > WorkspaceProtocol.
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.
It seemed important that the encode-to-url/decode-from-url logic be together. Previously encoding logic was in ScriptRequest and decoding was in WorkspaceUrlConnection which wasn't easy to follow. There doesn't seem to be a good OO way to encapsulate this.
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.
ScriptRequest was definitely the wrong place for the logic and I guess that no one is going to win an OO design prize when using the Java URL API...
OK, cool. Works for me now. Can you address the inline comments? |
Thanks. Added some comments. |
OK, it does the trick, I will merge it. Is there a corresponding JIRA issue? And can you add a line to the Release Notes? |
update workspace url to allow full job names
No JIRA. Release Notes updated. Thanks! |
Switched to use project.getFullName for URL so seed jobs can reside in folders, but also needed to encode the full name since it may contain slashes. Did some refactoring so the encode/decode logic would be in the same place.