[JENKINS-40718] Search by build params in build history widget #2683

Merged
merged 9 commits into from Mar 8, 2017

Conversation

3 participants
@szpak
Contributor

szpak commented Dec 29, 2016

In some kind of parameterized jobs (e.g. deploy to user selected environment) it would be useful to have an ability to search by build parameters values in a Build history widget.

By the way, a true case insensitivity of search parameters has been provided, more tests and some minor clean ups related to Java 7.

@oleg-nenashev

IMHO Build is a bad base class for it. It should be at least AbstractBuild.

I also think that the code should support other job types like Pipeline. It could be just a partial copy-paste of the getBuildVariables() method:

        Map<String,String> r = new HashMap<String, String>();

        ParametersAction parameters = getAction(ParametersAction.class);
        if (parameters!=null) {
            // this is a rather round about way of doing this...
            for (ParameterValue p : parameters) {
                String v = p.createVariableResolver(this).resolve(p.getName());
                if (v!=null) r.put(p.getName(),v);
            }
        }

        // allow the BuildWrappers to contribute additional build variables
        if (project instanceof BuildableItemWithBuildWrappers) {
            for (BuildWrapper bw : ((BuildableItemWithBuildWrappers) project).getBuildWrappersList())
                bw.makeBuildVariables(this,r);
        }
        return r;

I assume there is already such kind of implementation somewhere in the core. Maybe it worths generalization

@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Dec 30, 2016

Contributor

I changed it to AbstractBuild, however I don't fully understand a conception with copy-paste code from getBuildVariables(). In my code I already use that method:

    private boolean fitsSearchBuild(AbstractBuild runAsBuild) {
        Map buildVariables = runAsBuild.getBuildVariables();
        for (Object paramsValues : buildVariables.values()) {
            if (fitsSearchString(paramsValues)) {
                return true;
            }
        }
        return false;
    }

so I would assume that it handles also other type of jobs. If not, could you elaborate more how to implement that?

Contributor

szpak commented Dec 30, 2016

I changed it to AbstractBuild, however I don't fully understand a conception with copy-paste code from getBuildVariables(). In my code I already use that method:

    private boolean fitsSearchBuild(AbstractBuild runAsBuild) {
        Map buildVariables = runAsBuild.getBuildVariables();
        for (Object paramsValues : buildVariables.values()) {
            if (fitsSearchString(paramsValues)) {
                return true;
            }
        }
        return false;
    }

so I would assume that it handles also other type of jobs. If not, could you elaborate more how to implement that?

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Dec 30, 2016

Member

If the job is a ParameterizedJobMixin.ParameterizedJob, it can be any of its Runs.

This needs to be able to find builds in a Pipeline job, otherwise this will be weird.

Member

daniel-beck commented Dec 30, 2016

If the job is a ParameterizedJobMixin.ParameterizedJob, it can be any of its Runs.

This needs to be able to find builds in a Pipeline job, otherwise this will be weird.

@daniel-beck

This needs to be more general.

@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Jan 5, 2017

Contributor

@daniel-beck Could you tell me more how it could be implemented?

Contributor

szpak commented Jan 5, 2017

@daniel-beck Could you tell me more how it could be implemented?

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Jan 5, 2017

Member

Check all Runs for their ParametersAction's getAllParameters() and match those, rather (or in addition to) the AbstractBuild's getBuildVariables(). Possibly necessary to limit to ParameterValues that aren't isSensitive() or of specific types only, but this would be a start.

Member

daniel-beck commented Jan 5, 2017

Check all Runs for their ParametersAction's getAllParameters() and match those, rather (or in addition to) the AbstractBuild's getBuildVariables(). Possibly necessary to limit to ParameterValues that aren't isSensitive() or of specific types only, but this would be a start.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Jan 29, 2017

Member

https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/search/UserSearchProperty.java is the per-user option for search case sensitivity and should be honored by build filtering.

Member

daniel-beck commented Jan 29, 2017

https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/search/UserSearchProperty.java is the per-user option for search case sensitivity and should be honored by build filtering.

@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Jan 31, 2017

Contributor

@daniel-beck I've been able to get build parameters using ParametersAction which you suggested and it works also for WorkflowRun instances. I still need to polish some things, however 2 questions in the meantime.

  1. AbstractBuild getBuildVariables() seems to return the same as ParametersAction.getParameters() for a free style build. Is there a case for which I need to use AbstractBuild getBuildVariables() in addition to ParametersAction.getParameters()?
  2. Is it possible to specify and pass extra parameters between stages in the Pipeline job (e.g. the 1st stage generate a parameter which is passed to the seconds one)? I mean, do I need to get (somehow) stages (from WorkflowRun) and look for their parameters to make a match? Or WorkflowRun's parameters are the only paramters I'm interested in?
Contributor

szpak commented Jan 31, 2017

@daniel-beck I've been able to get build parameters using ParametersAction which you suggested and it works also for WorkflowRun instances. I still need to polish some things, however 2 questions in the meantime.

  1. AbstractBuild getBuildVariables() seems to return the same as ParametersAction.getParameters() for a free style build. Is there a case for which I need to use AbstractBuild getBuildVariables() in addition to ParametersAction.getParameters()?
  2. Is it possible to specify and pass extra parameters between stages in the Pipeline job (e.g. the 1st stage generate a parameter which is passed to the seconds one)? I mean, do I need to get (somehow) stages (from WorkflowRun) and look for their parameters to make a match? Or WorkflowRun's parameters are the only paramters I'm interested in?
@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Feb 1, 2017

Member

@szpak

  1. Probably Matrix Project axes.
  2. There's input steps, but I'd consider that to be out of scope. If it works with the basic Pipeline setup with ParametersProperty, that's good enough for now.
Member

daniel-beck commented Feb 1, 2017

@szpak

  1. Probably Matrix Project axes.
  2. There's input steps, but I'd consider that to be out of scope. If it works with the basic Pipeline setup with ParametersProperty, that's good enough for now.
@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Feb 1, 2017

Contributor

I applied your suggestions. The mechanism can deal with build parameters and build environments. It works with the pipeline jobs (I haven't tested with Matrix project axes). It takes into account also insensitiveSearch however I'm not sure how to set it in unit tests and therefore those tests are currently ignored. Feel free to review it again and propose further suggestions if needed.

Btw, some time ago I started an another thread about searching for nested view. @daniel-beck maybe you are able to suggest something also in that topic?

Btw2, I'm glad that Java 8 is coming to Jenkins. That code could be improved with the functional approach.

Contributor

szpak commented Feb 1, 2017

I applied your suggestions. The mechanism can deal with build parameters and build environments. It works with the pipeline jobs (I haven't tested with Matrix project axes). It takes into account also insensitiveSearch however I'm not sure how to set it in unit tests and therefore those tests are currently ignored. Feel free to review it again and propose further suggestions if needed.

Btw, some time ago I started an another thread about searching for nested view. @daniel-beck maybe you are able to suggest something also in that topic?

Btw2, I'm glad that Java 8 is coming to Jenkins. That code could be improved with the functional approach.

@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Feb 2, 2017

Contributor

The build error seems to be not releated to my changes:

Connection reset

Contributor

szpak commented Feb 2, 2017

The build error seems to be not releated to my changes:

Connection reset

@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Feb 10, 2017

Contributor

@daniel-beck @oleg-nenashev Do you have any other suggestions regarding thit PR?

Contributor

szpak commented Feb 10, 2017

@daniel-beck @oleg-nenashev Do you have any other suggestions regarding thit PR?

@daniel-beck daniel-beck removed the needs-fix label Feb 10, 2017

+ }
+
+ @Test
+ @Ignore //User with insensitiveSearch enabled needs to be injected

This comment has been minimized.

@daniel-beck

daniel-beck Feb 10, 2017

Member

Security380Test does most of what you need. Just impersonate any user name instead of anon, DummySecurityRealm allows all user names. User.get(…).addProperty(new UserSearchProperty(true)) should add the property to the user.

@daniel-beck

daniel-beck Feb 10, 2017

Member

Security380Test does most of what you need. Just impersonate any user name instead of anon, DummySecurityRealm allows all user names. User.get(…).addProperty(new UserSearchProperty(true)) should add the property to the user.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Feb 10, 2017

Member

LGTM except for the skipped tests.

Should be ready to go into 2.47 perhaps?

Member

daniel-beck commented Feb 10, 2017

LGTM except for the skipped tests.

Should be ready to go into 2.47 perhaps?

obsolete

@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Feb 10, 2017

Contributor

@daniel-beck It seems to be more complicated.

  1. I needed to duplicate multiple test classes/methods used to test the functionality in a unit way to tests module.
  2. I'm unable to get user properties. It fails in AllView on no primaryView set when I try to get the user with User.get("anonymous"). Looking in the code is seems that in tests MyViewsProperty is called with default constructor and that property in fact is not set.
java.lang.IllegalArgumentException: Argument for @Nonnull parameter 'primaryView' of hudson/model/AllView.migrateLegacyPrimaryAllViewLocalizedName must not be null

	at hudson.model.AllView.$$$reportNull$$$0(AllView.java)
	at hudson.model.AllView.migrateLegacyPrimaryAllViewLocalizedName(AllView.java)
	at hudson.model.MyViewsProperty.readResolve(MyViewsProperty.java:90)
	at hudson.model.MyViewsProperty.<init>(MyViewsProperty.java:78)
	at hudson.model.MyViewsProperty.<init>(MyViewsProperty.java:62)
	at hudson.model.MyViewsProperty$DescriptorImpl.newInstance(MyViewsProperty.java:217)
	at hudson.model.User.load(User.java:203)
	at hudson.model.User.<init>(User.java:155)
	at hudson.model.User.getOrCreate(User.java:463)
	at hudson.model.User.get(User.java:414)
	at hudson.model.User.get(User.java:373)
	at hudson.model.User.get(User.java:493)
	at jenkins.widgets.HistoryPageFilterUserTest$1.run(HistoryPageFilterUserTest.java:55)
	at hudson.security.ACL.impersonate(ACL.java:243)
	at jenkins.widgets.HistoryPageFilterUserTest.test_search_runs_by_build_result(HistoryPageFilterUserTest.java:49)
(...)

I'm not sure how I can workaround that.

Contributor

szpak commented Feb 10, 2017

@daniel-beck It seems to be more complicated.

  1. I needed to duplicate multiple test classes/methods used to test the functionality in a unit way to tests module.
  2. I'm unable to get user properties. It fails in AllView on no primaryView set when I try to get the user with User.get("anonymous"). Looking in the code is seems that in tests MyViewsProperty is called with default constructor and that property in fact is not set.
java.lang.IllegalArgumentException: Argument for @Nonnull parameter 'primaryView' of hudson/model/AllView.migrateLegacyPrimaryAllViewLocalizedName must not be null

	at hudson.model.AllView.$$$reportNull$$$0(AllView.java)
	at hudson.model.AllView.migrateLegacyPrimaryAllViewLocalizedName(AllView.java)
	at hudson.model.MyViewsProperty.readResolve(MyViewsProperty.java:90)
	at hudson.model.MyViewsProperty.<init>(MyViewsProperty.java:78)
	at hudson.model.MyViewsProperty.<init>(MyViewsProperty.java:62)
	at hudson.model.MyViewsProperty$DescriptorImpl.newInstance(MyViewsProperty.java:217)
	at hudson.model.User.load(User.java:203)
	at hudson.model.User.<init>(User.java:155)
	at hudson.model.User.getOrCreate(User.java:463)
	at hudson.model.User.get(User.java:414)
	at hudson.model.User.get(User.java:373)
	at hudson.model.User.get(User.java:493)
	at jenkins.widgets.HistoryPageFilterUserTest$1.run(HistoryPageFilterUserTest.java:55)
	at hudson.security.ACL.impersonate(ACL.java:243)
	at jenkins.widgets.HistoryPageFilterUserTest.test_search_runs_by_build_result(HistoryPageFilterUserTest.java:49)
(...)

I'm not sure how I can workaround that.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Feb 10, 2017

Member

@szpak Right, needs to be a test in test/src/test and have a JenkinsRule. Move your test cases over there.

Member

daniel-beck commented Feb 10, 2017

@szpak Right, needs to be a test in test/src/test and have a JenkinsRule. Move your test cases over there.

@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Feb 10, 2017

Contributor

@daniel-beck I already did it (making a lot of duplication), however it fails with the exception I mentioned (even with the @Rule - Jenkins instance is initialized).

I put the code to the separate branch - there is something obvious missing:
https://github.com/szpak/jenkins/blob/4515fc31b3e17e7b88e8714de9a2dcb141253efa/test/src/test/java/jenkins/widgets/HistoryPageFilterUserTest.java#L54

Contributor

szpak commented Feb 10, 2017

@daniel-beck I already did it (making a lot of duplication), however it fails with the exception I mentioned (even with the @Rule - Jenkins instance is initialized).

I put the code to the separate branch - there is something obvious missing:
https://github.com/szpak/jenkins/blob/4515fc31b3e17e7b88e8714de9a2dcb141253efa/test/src/test/java/jenkins/widgets/HistoryPageFilterUserTest.java#L54

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Feb 10, 2017

Member

@szpak mvn -Dtest=jenkins.widgets.HistoryPageFilterUserTest -DfailIfNoTests=false verify worked for me on 4515fc3, what am I missing?

Member

daniel-beck commented Feb 10, 2017

@szpak mvn -Dtest=jenkins.widgets.HistoryPageFilterUserTest -DfailIfNoTests=false verify worked for me on 4515fc3, what am I missing?

@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Feb 10, 2017

Contributor
Contributor

szpak commented Feb 10, 2017

@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Feb 12, 2017

Contributor

@daniel-beck It works fine with Maven and today also with Idea...

I would prefer to leave unit tests in core (where they currently are). I stripped down integration tests to reduce duplication between modules.

Contributor

szpak commented Feb 12, 2017

@daniel-beck It works fine with Maven and today also with Idea...

I would prefer to leave unit tests in core (where they currently are). I stripped down integration tests to reduce duplication between modules.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Feb 12, 2017

Member

@oleg-nenashev Needs a re-review from you.

Member

daniel-beck commented Feb 12, 2017

@oleg-nenashev Needs a re-review from you.

@oleg-nenashev

I still think that UserSearchProperty should not be a variable for every kind of search, but seems I've been outvoted on this topic in another PR.

Other code looks good to me

+ if (data instanceof Number) {
+ return data.toString().equals(searchString);
+ } else {
+ if (UserSearchProperty.isCaseInsensitive()) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Feb 12, 2017

Member

Usage of this class for search in all places still confuses me, but IIRC I've been outvoted in another PR

@oleg-nenashev

oleg-nenashev Feb 12, 2017

Member

Usage of this class for search in all places still confuses me, but IIRC I've been outvoted in another PR

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Feb 12, 2017

Member

Thanks.

Let's hold for another week.

Member

daniel-beck commented Feb 12, 2017

Thanks.

Let's hold for another week.

@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Feb 12, 2017

Contributor

Btw, it's not directly related to that PR, but I really wonder why searching is case-sensitive by default. It's unintuitive for me. I prefer to see more results and try to find a way to limit them if needed (by looking for that property). Is there any good argument to have a default value to be case-sensitive?

I haven't heard about that property and after my modifications (taking that flag into account) people will be surprised that searching by failure stopped working/doesn't work (new Jenkins users would probably assume that searching by a job status is not supported - as failure or success would return nothing by default).

Contributor

szpak commented Feb 12, 2017

Btw, it's not directly related to that PR, but I really wonder why searching is case-sensitive by default. It's unintuitive for me. I prefer to see more results and try to find a way to limit them if needed (by looking for that property). Is there any good argument to have a default value to be case-sensitive?

I haven't heard about that property and after my modifications (taking that flag into account) people will be surprised that searching by failure stopped working/doesn't work (new Jenkins users would probably assume that searching by a job status is not supported - as failure or success would return nothing by default).

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Mar 8, 2017

Member

@szpak

Let's hold for another week.

Oops. Sorry about that.

@oleg-nenashev WDYT, merge this week? Seems ready to me.

Member

daniel-beck commented Mar 8, 2017

@szpak

Let's hold for another week.

Oops. Sorry about that.

@oleg-nenashev WDYT, merge this week? Seems ready to me.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
Member

oleg-nenashev commented Mar 8, 2017

@daniel-beck Yes, LGTM

@szpak

This comment has been minimized.

Show comment
Hide comment
@szpak

szpak Mar 8, 2017

Contributor

Btw, I started a discussion about case (in)sensitivity by default in the search mechanism (in general) on the mailing list. However, it doesn't impact that (long waiting) issue/PR directly.

Contributor

szpak commented Mar 8, 2017

Btw, I started a discussion about case (in)sensitivity by default in the search mechanism (in general) on the mailing list. However, it doesn't impact that (long waiting) issue/PR directly.

@daniel-beck daniel-beck merged commit 62adfa8 into jenkinsci:master Mar 8, 2017

2 checks passed

Jenkins This pull request looks good
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@szpak szpak deleted the szpak:feature/JENKINS-40718-searchByBuildParams branch Mar 8, 2017

oleg-nenashev added a commit to oleg-nenashev/jenkins.io that referenced this pull request Mar 9, 2017

@oleg-nenashev oleg-nenashev referenced this pull request in jenkins-infra/jenkins.io Mar 9, 2017

Merged

Noting changes towards 2.50 #725

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment