[JENKINS-36282] Add support for exporting jobs in folders recursively #1
Conversation
|
|
||
| private Map<String, Collection<TopLevelItem>> getItemsRecursive(String namePrefix, Collection<TopLevelItem> items) { | ||
| Map<String, Collection<TopLevelItem>> result = new HashMap<>(); | ||
| List<TopLevelItem> currentLevelItems = new ArrayList<>(); |
There was a problem hiding this comment.
Depending on the number of folders and projects on the Jenkins instance, and how sparse the folders are, these ArrayList and HashMap objects might have a lot of unused space. Does anyone think I should make them a smaller size to start, or use a LinkedList instead or anything?
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
|
This wiki page would need to be updated as well. |
|
It's probably a correct fix, but it would be great to know @daniel-beck's plans about releasing the plugin. So far it is not released, and the issue probably needs to be fixed in the core |
|
Yes it requires that JENKINS-40750 be completed. I'm not sure if the current PR to remove cc.xml from core is waiting for a new LTS release or anything. |
|
It just hangs since @daniel-beck has lots of other things to do, I'd guess. Maybe a correct approach is to fix the issue in the core now and then to integrate this PR here so that the fix will get applied after detaching. If you consider it as an issue requiring backporting, then the fix needs to submitted against the core for sure. |
|
I expect this will kick me into finally acting on this in the next few days. Shouldn't be too much work. |
|
@daniel-beck I am happy to help write any wiki pages or documentation that is needed to release this plugin. |
| * as well. The map is keyed by the folder the item is in, with top-level | ||
| * items having an empty key. | ||
| */ | ||
| public Map<String, Collection<TopLevelItem>> getItems() { |
There was a problem hiding this comment.
If it is considered as a backportable bugfix, new public methods should be marked as @Restricted(NoExternalUse.class)
| for (TopLevelItem i : items) { | ||
| if (i instanceof ItemGroup) { | ||
| ItemGroup g = (ItemGroup) i; | ||
| result.putAll(getItemsRecursive(namePrefix + g.getDisplayName() + "/", g.getItems())); |
There was a problem hiding this comment.
I am not sure about Display Name here. It causes some concerns since you duplicate the core's logic here. I would rather suggest to use Item#getFullDisplayName though it decreases the performance of the call
There was a problem hiding this comment.
I didn't know about Item#getFullDisplayName, so I will update it to use that.
| @@ -28,19 +28,21 @@ THE SOFTWARE. | |||
| --> | |||
| <?jelly escape-by-default='true'?> | |||
| <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form" xmlns:i="jelly:fmt"> | |||
| <st:contentType value="text/xml;charset=UTF-8" /> | |||
| <st:contentType value="application/xml;charset=UTF-8" /> | |||
There was a problem hiding this comment.
According to RFC referenced in https://stackoverflow.com/questions/4832357/whats-the-difference-between-text-xml-vs-application-xml-for-webservice-respons . I would argue that "text/xml" was correct since the output is really "readable by casual users". It is also not related to the bug from what I see
There was a problem hiding this comment.
You're right, I changed it to make the testing logic simpler as JenkinsRule#createWebClient#goToXml requires application/xml (Maybe that method should also accept text/xml?). I will change it back and update the tests.
…content type to text/xml
| for (TopLevelItem i : items) { | ||
| if (i instanceof ItemGroup) { | ||
| ItemGroup g = (ItemGroup) i; | ||
| result.addAll(getItemsRecursive(g.getItems())); |
There was a problem hiding this comment.
passing target array as an argument would make the method much more efficient from the memory allocation perspective. Now you allocate a new array list for every Folder/ItemGroup just to move them to another array on the upper level. And so on
There was a problem hiding this comment.
Yes that's a very good point, I'll fix it. Thanks!
|
@reviewbybees done |
|
This went into core with some changes as jenkinsci/jenkins#3060 and jenkinsci/jenkins#3081. This PR would need to be modified to match those fixes instead. |
JENKINS-36282
I added the ability to specify a query parameter to list all jobs recursively in cc.xml. I made it optional in case someone relies on the current behavior, but if that isn't an issue we could just make the recursive behavior a non-optional default.
The name of jobs in folders are prefixed by the folder name, i.e.
<Project ... name="Folder1/Project1">, so I believe this also fixes JENKINS-34641.@reviewbybees