-
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
package javaposse.jobdsl.plugin | ||
|
||
import hudson.FilePath | ||
import hudson.model.AbstractBuild | ||
import hudson.model.AbstractProject | ||
import jenkins.model.Jenkins | ||
|
||
class WorkspaceProtocol { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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... |
||
|
||
/** | ||
* Create a workspace URL that represents the base dir of the given AbstractProject. | ||
*/ | ||
static URL createWorkspaceUrl(AbstractProject project) { | ||
String jobName = project.fullName | ||
String encodedJobName = URLEncoder.encode(jobName, "UTF-8") | ||
new URL(null, "workspace://$encodedJobName/", new WorkspaceUrlHandler()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is encoding a path into the domain part a good URL format? This will generate URLs like workspace://foo%2Fbar/. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously it just used the short name which breaks when in a folder. We need to pass the full path somehow and this seems to do the trick. |
||
} | ||
|
||
/** | ||
* Create a workspace URL that represents the given FilePath. | ||
*/ | ||
static URL createWorkspaceUrl(AbstractBuild build, FilePath filePath) { | ||
String relativePath = filePath.getRemote() - build.workspace.getRemote() | ||
new URL(createWorkspaceUrl(build.project), "$relativePath/", new WorkspaceUrlHandler()) | ||
} | ||
|
||
/** | ||
* Parse the AbstractProject from the given workspace URL representation. | ||
*/ | ||
static AbstractProject getProjectFromWorkspaceUrl(URL url) { | ||
Jenkins jenkins = Jenkins.instance | ||
if (!jenkins) { | ||
throw new IllegalStateException("Not in a running Jenkins") | ||
} | ||
|
||
String jobName = url.host | ||
String decodedJobName = URLDecoder.decode(jobName, "UTF-8") | ||
(AbstractProject) Jenkins.instance.getItemByFullName(decodedJobName) | ||
} | ||
|
||
/** | ||
* Parse the FilePath from the given workspace URL representation. | ||
*/ | ||
static FilePath getFilePathFromUrl(URL url) { | ||
AbstractProject project = getProjectFromWorkspaceUrl(url) | ||
FilePath workspace = project.someWorkspace | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why someWorkspace, shouldn't it be the currentWorkspace? Does this have any side effects? Add at least a comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was existing code I moved from ScriptRequest. |
||
String relativePath = url.file[1..-1] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also add a comment here, it's not obvious. |
||
workspace.child relativePath | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,35 +1,102 @@ | ||
package javaposse.jobdsl.plugin | ||
|
||
import javaposse.jobdsl.dsl.DslScriptLoader | ||
import javaposse.jobdsl.dsl.FileJobManagement | ||
import javaposse.jobdsl.dsl.JobManagement | ||
import javaposse.jobdsl.dsl.ScriptRequest | ||
import com.cloudbees.hudson.plugins.folder.Folder | ||
import hudson.FilePath | ||
import hudson.model.AbstractBuild | ||
import hudson.model.AbstractProject | ||
import hudson.model.FreeStyleProject | ||
import org.junit.Rule | ||
import org.jvnet.hudson.test.JenkinsRule | ||
import spock.lang.Specification | ||
|
||
class WorkspaceProtocolSpec extends Specification { | ||
class WorkspaceProtocolSpec extends Specification { | ||
|
||
@Rule | ||
public JenkinsRule jenkinsRule = new JenkinsRule() | ||
|
||
def 'url for project'() { | ||
given: | ||
AbstractProject project = jenkinsRule.createFreeStyleProject('test-project') | ||
|
||
when: | ||
URL url = WorkspaceProtocol.createWorkspaceUrl(project) | ||
|
||
then: | ||
url.host == 'test-project' | ||
url.file == '/' | ||
|
||
when: | ||
AbstractProject returnedProject = WorkspaceProtocol.getProjectFromWorkspaceUrl(url) | ||
|
||
then: | ||
project == returnedProject | ||
} | ||
|
||
def 'url for project in folder'() { | ||
given: | ||
Folder folder = jenkinsRule.jenkins.createProject Folder, 'folder' | ||
AbstractProject project = folder.createProject FreeStyleProject, 'test-project' | ||
|
||
when: | ||
URL url = WorkspaceProtocol.createWorkspaceUrl(project) | ||
|
||
then: | ||
url.host == 'folder%2Ftest-project' | ||
url.file == '/' | ||
|
||
when: | ||
AbstractProject returnedProject = WorkspaceProtocol.getProjectFromWorkspaceUrl(url) | ||
|
||
then: | ||
project == returnedProject | ||
} | ||
|
||
def 'url for project with file'() { | ||
given: | ||
AbstractProject project = jenkinsRule.createFreeStyleProject('test-project') | ||
AbstractBuild build = project.createExecutable() | ||
FilePath workspace = jenkinsRule.jenkins.getWorkspaceFor(project) | ||
build.setWorkspace workspace | ||
FilePath filePath = workspace.child('files') | ||
|
||
def 'load workspace url'() { | ||
when: | ||
URL url = new URL(null, "workspace://JOB/dir/file.dsl", new WorkspaceUrlHandler()) | ||
URL url = WorkspaceProtocol.createWorkspaceUrl(build, filePath) | ||
|
||
then: | ||
url.host == 'JOB' | ||
url.file == '/dir/file.dsl' | ||
url.host == 'test-project' | ||
url.file == '/files/' | ||
|
||
when: | ||
AbstractProject returnedProject = WorkspaceProtocol.getProjectFromWorkspaceUrl(url) | ||
FilePath returnedFilePath = WorkspaceProtocol.getFilePathFromUrl(url) | ||
|
||
then: | ||
project == returnedProject | ||
filePath == returnedFilePath | ||
} | ||
|
||
def 'reference workspace form dsl'() { | ||
def resourcesDir = new File("src/test/resources") | ||
JobManagement jm = new FileJobManagement(resourcesDir) | ||
URL url = new URL(null, "workspace://JOB/dir/file.dsl", new WorkspaceUrlHandler()) | ||
def 'url for project in folder with file'() { | ||
given: | ||
Folder folder = jenkinsRule.jenkins.createProject Folder, 'folder' | ||
AbstractProject project = folder.createProject FreeStyleProject, 'test-project' | ||
AbstractBuild build = project.createExecutable() | ||
FilePath workspace = jenkinsRule.jenkins.getWorkspaceFor(project) | ||
build.setWorkspace workspace | ||
FilePath filePath = workspace.child('files') | ||
|
||
setup: | ||
ScriptRequest request = new ScriptRequest('caller.dsl', null, url); | ||
when: | ||
URL url = WorkspaceProtocol.createWorkspaceUrl(build, filePath) | ||
|
||
then: | ||
url.host == 'folder%2Ftest-project' | ||
url.file == '/files/' | ||
|
||
when: | ||
DslScriptLoader.runDslEngine(request, jm) | ||
AbstractProject returnedProject = WorkspaceProtocol.getProjectFromWorkspaceUrl(url) | ||
FilePath returnedFilePath = WorkspaceProtocol.getFilePathFromUrl(url) | ||
|
||
then: | ||
thrown(IllegalStateException) | ||
project == returnedProject | ||
filePath == returnedFilePath | ||
} | ||
} |
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.