Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[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 38412a9
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 12 deletions.
12 changes: 4 additions & 8 deletions core/src/main/java/hudson/cli/ListJobsCommand.java
Expand Up @@ -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;

Expand Down Expand Up @@ -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 {
Expand Down
68 changes: 64 additions & 4 deletions core/src/test/java/hudson/cli/ListJobsCommandTest.java
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down

3 comments on commit 38412a9

@daniel-beck
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@jglick jglick commented on 38412a9 Mar 20, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Please sign in to comment.