Skip to content
Permalink
Browse files

[fixed JENKINS-18393] Do not list any ItemGroup in list-jobs CLI command

List item group recursively.
  • Loading branch information...
olivergondza committed Jun 22, 2013
1 parent 20391ab commit 38412a94555a18ddb97fc5d9dcd78e983bb3856f
@@ -24,16 +24,13 @@
package hudson.cli;

import java.util.Collection;
import java.util.LinkedHashSet;

import java.util.Collections;

import hudson.model.Item;
import hudson.model.ItemGroup;
import hudson.model.Items;
import hudson.model.TopLevelItem;
import hudson.model.ViewGroup;
import hudson.model.View;
import hudson.Extension;
import jenkins.model.ModifiableTopLevelItemGroup;
import jenkins.model.Jenkins;
import org.kohsuke.args4j.Argument;

@@ -68,9 +65,8 @@ protected int run() throws Exception {
final Item item = h.getItemByFullName(name);

// If item group was found use it's jobs.
if (item instanceof ItemGroup) {
ItemGroup itemGroup = (ItemGroup) item;
jobs = itemGroup.getItems();
if (item instanceof ModifiableTopLevelItemGroup) {
jobs = Items.getAllItems((ModifiableTopLevelItemGroup) item, TopLevelItem.class);
}
// No view and no item group with the given name found.
else {
@@ -2,30 +2,46 @@

import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.powermock.api.mockito.PowerMockito.mock;
import static org.powermock.api.mockito.PowerMockito.mockStatic;
import static org.powermock.api.mockito.PowerMockito.when;
import hudson.matrix.MatrixConfiguration;
import hudson.matrix.MatrixProject;
import hudson.model.AbstractProject;
import hudson.model.Item;
import hudson.model.ItemGroup;
import hudson.model.TopLevelItem;
import hudson.model.TopLevelItemDescriptor;
import hudson.model.ViewGroup;
import hudson.model.ViewTest.CompositeView;
import hudson.model.View;

import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.PrintStream;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;

import javax.annotation.CheckForNull;
import javax.servlet.ServletException;

import jenkins.model.Jenkins;
import jenkins.model.ModifiableTopLevelItemGroup;

import org.hamcrest.Description;
import org.hamcrest.TypeSafeMatcher;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.StaplerResponse;
import org.mockito.Mockito;
import org.powermock.core.classloader.annotations.PrepareForTest;
import org.powermock.core.classloader.annotations.SuppressStaticInitializationFor;
@@ -53,14 +69,58 @@ public void setUp() {
}

@Test
public void getNullForNonexistingName() throws Exception {
public void failForNonexistingName() throws Exception {

when(jenkins.getView(null)).thenReturn(null);
when(jenkins.getItemByFullName(null)).thenReturn(null);
when(jenkins.getView("NoSuchViewOrItemGroup")).thenReturn(null);
when(jenkins.getItemByFullName("NoSuchViewOrItemGroup")).thenReturn(null);

assertThat(runWith("NoSuchViewOrItemGroup"), equalTo(-1));
assertThat(stdout, is(empty()));
assertThat(stderr, is(not(empty())));
assertThat(stderr.toString(), containsString("No view or item group with the given name found"));
}

@Test
public void failForMatrixProject() throws Exception {

final MatrixProject matrix = mock(MatrixProject.class);
final MatrixConfiguration config = mock(MatrixConfiguration.class);
when(matrix.getItems()).thenReturn(Arrays.asList(config));

when(jenkins.getView("MatrixJob")).thenReturn(null);
when(jenkins.getItemByFullName("MatrixJob")).thenReturn(matrix);

assertThat(runWith("MatrixJob"), equalTo(-1));
assertThat(stdout, is(empty()));
assertThat(stderr.toString(), containsString("No view or item group with the given name found"));
}

@Test
public void getAllJobsFromFolders() throws Exception {

abstract class Folder implements ModifiableTopLevelItemGroup, TopLevelItem {
}

final Folder folder = mock(Folder.class);
final Folder nestedFolder = mock(Folder.class);
when(folder.getDisplayName()).thenReturn("Folder");
when(nestedFolder.getDisplayName()).thenReturn("NestedFolder");

final TopLevelItem job = job("job");
final TopLevelItem nestedJob = job("nestedJob");
when(job.hasPermission(Item.READ)).thenReturn(true);
when(nestedJob.hasPermission(Item.READ)).thenReturn(true);
when(job.getRelativeNameFrom((ItemGroup<TopLevelItem>) folder)).thenReturn("job");
when(nestedJob.getRelativeNameFrom((ItemGroup<TopLevelItem>) folder)).thenReturn("nestedJob");

when(folder.getItems()).thenReturn(Arrays.asList(nestedFolder, job));
when(nestedFolder.getItems()).thenReturn(Arrays.asList(nestedJob));

when(jenkins.getView("OuterFolder")).thenReturn(null);
when(jenkins.getItemByFullName("OuterFolder")).thenReturn(folder);

assertThat(runWith("OuterFolder"), equalTo(0));
assertThat(stdout, listsJobs("job", "nestedJob"));

This comment has been minimized.

Copy link
@jglick

jglick Mar 19, 2014

Member

This seems wrong to me. I would expect it to return either [job, NestedFolder/nestedJob], or (preferably) just [job]. For the first, ListJobsCommand should print getRelativeNameFrom rather than getName. (Or getDisplayName; cf. aaa5001, currently in validated merge.) For the second, it should use ItemGroup.getItems and Util.filter the result, rather than using Items.getAllItems.

Probably the first change needs to be done even if the second was also done, so that if you list jobs in a view which includes jobs from nested folders, they are printed using paths relative to the View.ownerItemGroup.

This comment has been minimized.

Copy link
@olivergondza

olivergondza Mar 20, 2014

Author Member

I am not sure I follow you here. Recursive job listing was introduced by #793. I prefer no to revert it without replacement.

As long as output format is concerned, it makes sense to me print whatever build (and *-job) commands accept on input (option handler uses Jenkins.getItemByFullName). Though, I am not sure current implementation actually generate such output.

I will move this to test-harness and make it an end-to-end test so we can enforce this.

This comment has been minimized.

Copy link
@jglick

jglick Mar 20, 2014

Member

#793 at first seemed to be about showing jobs in nested views. But here it is also showing jobs in nested folders, which is quite a different scenario.

The current implementation prints only a simple name for all jobs, so definitely it could not be fed as is into other CLI commands. But my proposal was to emit a relative path so you could at least prepend the known folder path.

assertThat(stderr, is(empty()));
}

@Test

3 comments on commit 38412a9

@daniel-beck

This comment has been minimized.

Copy link
Member

replied Mar 20, 2014

It seems to me that list-jobs is at a dead end unless we break backwards compatibility. Maybe gather the issues with this command, and write a new, more flexible list-items? We could revert the compatibility breaking aaa5001 then as well.

Is there any concept of deprecating CLI commands? E.g. if they're @Deprecated, hide from the commands list? (If not, there should be!)

@jglick

This comment has been minimized.

Copy link
Member

replied Mar 20, 2014

Creating a replacement command is also an option. There is no particular system for deprecating a command, though I suppose HelpCommand could do what you are proposing.

@olivergondza

This comment has been minimized.

Copy link
Member Author

replied Mar 22, 2014

@jglick, @daniel-beck: I agree let's deprecate it. Please review JENKINS-22301 and #1164.

Please sign in to comment.
You can’t perform that action at this time.