-
Notifications
You must be signed in to change notification settings - Fork 661
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
Capture Jenkins job names correctly when the folder plugin is being used #42
Conversation
@@ -73,7 +73,24 @@ class InfoController { | |||
throw new MasterNotFoundException("Master '${master}' does not exist") | |||
} | |||
|
|||
jenkinsClient.jobs.list.collect { it.name } | |||
def jobList = [] |
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 like to see a corresponding test in https://github.com/spinnaker/igor/blob/master/igor-web/src/test/groovy/com/netflix/spinnaker/igor/jenkins/InfoControllerSpec.groovy
This is failing for me when I am just trying to get the job details after installing the jenkins folders plugin on a jenkins host. There doesn't seem to be a straightforward way to modify spring paths to accept / I have this job
under the host jenkins1. When I try to get the details for it, it doesn't appear to handle the path correctly: http://localhost:8080/jobs/jenkins1/Jenkins%20folder/job/hakuna%20matata |
} | ||
|
||
@RequestMapping(value = '/jobs/{master}/{job:.+}') | ||
JobConfig getJobConfig(@PathVariable String master, @PathVariable String job) { | ||
@RequestMapping(value = '/jobs/{master}/**') |
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.
In order to allow forward slashes in the names of Jenkins jobs, we can manually parse the URL like this. If this looks acceptable, I can do something similar in the BuildController
. I'll have to move the job names to the end of all those URLs and manually parse them in this same way.
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 probably prefer to move job name as a query parameter like some other people have suggested in the comments.
To be clear, these are the URL changes I've made. Note that the job name has to be at the end of the URL in order to allow any arbitrary number of forward slashes in the name.
|
changes look sensible. Upstream services would need to change to reflect these - |
This looks to me like a breaking API change? If so, we'll need to version the API in here (or perhaps just leave old and new running side-by-side). |
@ajordens, doesn't Gate handle talking to Igor? It seems like users shouldn't be working directly with Igor's API. I guess it makes sense to version it either way. Gate's URLs are changing as well (spinnaker/gate#161), so we'll need to address that issue. But yes, this is a breaking API change. If you'd like to version the API, how would you prefer to handle that? Maybe just prepend the new URLs with |
if we leave old and new running side by side, the UI will not be able to handle jobs with the folders plugin. My vote is to change the gate API so that it has v2 attached to it. Deck would need to be updated as well to support the new url patterns https://github.com/spinnaker/deck/blob/master/app/scripts/modules/core/ci/jenkins/igor.service.js It also seems that bakery parses jobs incorrectly - https://github.com/spinnaker/orca/blob/master/orca-core/src/main/groovy/com/netflix/spinnaker/orca/pipeline/util/PackageInfo.groovy#L120-L124 so this would have to change as well. The last bit to sanity check is how things are constructed to link back to the build in the clusters view in deck. I suspect this might be fine but basically we need to wire up all those pieces and test it works correctly. |
might be useful to note that I'm just launching a new jenkins via |
If encoding paths/filenames is an issue, is it possible to switch to making them query parameters? I think that would encode things. Not 100% sure how to run that through retrofit. I know Spring MVC encodes stuff, but I can't say the same for retrofit. If you're breaking the API anyway, this might be the least risky way to get the entire value over the wire. |
Updated gate to prepend the new URLs with |
Deck code here - spinnaker/deck#1766 |
97ca888
to
da853ce
Compare
* Jobs created with the Jenkins Folders plugin are nested. | ||
* This query looks for jobs within folders with a depth of 10. | ||
*/ | ||
@GET('/api/xml?tree=jobs[name,jobs[name,jobs[name,jobs[name,jobs[name,jobs[name,jobs[name,jobs[name,jobs[name,jobs[name]]]]]]]]]]') |
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.
in the ideal world this kind of structure and resolution would be configurable at the jenkins masters level.
5274a8b
to
fec2099
Compare
Getting job configuration from deck UI does not work for folders. Create a job at the level folder1/folder2/folderjob Make the job parametrized and add a few parameters to the job. In deck, create a new application -> new pipeline -> new jenkins stage Select the master for the folder job and select this job. Seems like the slashes get encoded when trying to get the job configuration: http://192.168.99.100:8084/v2/builds/folders/jobs/folder1%2Fjob%2Ffolder2%2Fjob%2Ffolderjob The result of this is that parameters for the job are not shown |
Polling is broken for jobs coming from the folder plugin. The BuildMonitor in igor gets the list of jobs via the getProjects endpoint in JenkinsClient. This information is then used to resolve if there is a new build or not and new events get sent to Echo to trigger pipelines. The path is here - igor/igor-web/src/main/groovy/com/netflix/spinnaker/igor/jenkins/client/JenkinsClient.groovy Lines 40 to 41 in 78421f8
Seems that for jobs with folders attached to it, only the top level folder is displayed as an available job. The same change to get a list of jobs needs to be made so that new build events from jobs get logged correctly. |
I've addressed the issue with polling jobs. While testing, I also noticed that job names with spaces were causing issues because I had marked the URI as encoded to avoid encoding the forward slashes. I'll need to look over the other PRs to ensure special characters like spaces are correctly encoded. |
Other PRs have been updated to correctly handle spaces in job names. I believe this is again ready for review. |
do you mind squashing your commits? |
9ff17df
to
d7e5318
Compare
Squashed and rebased all PRs |
I know we talked about this ... but this is a breaking change and the internal Spinnaker callers of igor (gate and orca) are going to break (even if for only an optimal 5 minutes as everything is rolled out). @AMeng I realize the there are PRs against orca and gate, but we deploy each service independently and strive to avoid dependent rollouts. @tomaslin What do you figure the LOE would be to re-insert the old igor endpoints? As simple as restoring the code? |
If it is a requirement that these changes be deployable separately, then Gate will have the same issues. I've prepended the new gate URLs with |
@ajordens I've re-added the original endpoints for both Igor and Gate. I tried to make it as easy as possible to rip them back out whenever you think its ok to do so (both the methods and the tests are grouped on the bottom). |
I've tested this PR and with the other changes against a jenkins slave without the folders plugin and one with it. Job names with and without spaces work correctly in both and triggering works as well.. LGTM. @ajordens can you run through the API changes and merge if you're satisfied? |
Capture Jenkins job names correctly when the folder plugin is being used
This is meant to address spinnaker/spinnaker#605.
I'm able to hit
http://localhost:8080/jobs/<JenkinsName>/
and see a list of jobs with the expected names:Its also likely that a few endpoints will need to be updated since they currently have the job names in the middle of other parameters, such as "/jobs/{jobName}/builds" which could be troublesome.