Skip to content
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

[JENKINS-24141] Pull ChangeLogSet-related logic out of AbstractBuild #2730

Merged
merged 20 commits into from May 10, 2017

Conversation

5 participants
@abayer
Copy link
Member

commented Jan 31, 2017

JENKINS-24141

  • Make sure implementation is sane.
  • SCM.buildEnvVars needs to be changed to Run.
  • Port DependencyGraph as well? (questionable whether we want to do this - feedback desired) (spoiler - we don't want to do this, at least not here - this is a major change I don't think should be in this scope)

Changelog

  • RFE: Generalize the changelog API to support non-AbstractBuild Run Types

Upstream of:

cc @reviewbybees esp @jglick

[WIP JENKINS-24141] First work on abstracting out changelogs
Need to add tests, but first need to determine if this actually makes
sense as I've implemented it.

@jglick jglick self-assigned this Jan 31, 2017

@jglick
Copy link
Member

left a comment

Looking basically right so far.

Do you plan to also port DependencyGraph stuff (upstream/downstream, used tangentially by SCM-related code)?

@@ -637,7 +639,7 @@ public Calendar getLastChange() {
}

@Exported
public AbstractProject getProject() {
public Job<?,?> getProject() {

This comment has been minimized.

Copy link
@jglick

jglick Jan 31, 2017

Member

Binary-incompatible, I am afraid, so you will need to do something like

@Deprecated
public AbstractProject getProject() {
    return project instanceof AbstractProject ? (AbstractProject) project : null;
}
@Exported(name="project")
public Job<?,?> getProject() {
    return project;
}

This comment has been minimized.

Copy link
@abayer

abayer Jan 31, 2017

Author Member

Okiedokie.

modified.add(user);
if (r instanceof RunWithSCM) {
RunWithSCM runWithSCM = (RunWithSCM) r;
for (ChangeLogSet<? extends ChangeLogSet.Entry> c : runWithSCM.getChangeSets()) {

This comment has been minimized.

Copy link
@jglick

jglick Jan 31, 2017

Member

Tip: to minimize diff hunk size for reviewers (and thus reduce chance of merge conflicts, git blame breakage, etc. etc.), you can keep the original indentation without violating code style:

for (Run<?,?> r : runs) {
    if (canceled()) {
        return;
    }
    if (!(r instanceof RunWithSCM)) {
        continue;
    }
    for (ChangeLogSet.Entry entry : ((RunWithSCM) r).getChangeSet()) {
        // …rest untouched

This comment has been minimized.

Copy link
@abayer

abayer Jan 31, 2017

Author Member

Will note for future reference.

}
}
}
// TODO consider also adding the user of the UserCause when applicable
buildCount++;
// TODO this defeats lazy-loading. Should rather do a breadth-first search, as in hudson.plugins.view.dashboard.builds.LatestBuilds
// (though currently there is no quick implementation of RunMap.size() ~ idOnDisk.size(), which would be needed for proper progress)
progress((itemCount + 1.0 * buildCount / builds.size()) / (items.size() + 1));
progress((itemCount + 1.0 * buildCount / runs.size()) / (items.size() + 1));

This comment has been minimized.

Copy link
@jglick

jglick Jan 31, 2017

Member

Avoid gratuitous renames to minimize diff hunks.

This comment has been minimized.

Copy link
@abayer

abayer Jan 31, 2017

Author Member

Mea culpa. =)

import java.util.Set;

/**
* @since 2.xxx

This comment has been minimized.

Copy link
@jglick

jglick Jan 31, 2017

Member

use FIXME

This comment has been minimized.

Copy link
@abayer

abayer Jan 31, 2017

Author Member

Wasn't sure what to do there - perfect.


@Nonnull List<ChangeLogSet<? extends ChangeLogSet.Entry>> getChangeSets();

@Nonnull Set<User> getCulprits();

This comment has been minimized.

Copy link
@jglick

jglick Jan 31, 2017

Member

I think this would be better done as a mixin so that we do not need to copy implementations of getCulprits and hasParticipant.

This comment has been minimized.

Copy link
@abayer

abayer Jan 31, 2017

Author Member

Was wondering about that - I decided to not go that far initially but now I will do so!

This comment has been minimized.

Copy link
@abayer

abayer Jan 31, 2017

Author Member

But fwiw, getCulprits actually will differ, since AbstractBuild.getCulprits() relies on AbstractBuild.getDependencyChanges(...), which isn't functionality that I think makes sense in WorkflowRun. Especially since the Fingerprinter stuff is allllll about AbstractProject.

This comment has been minimized.

Copy link
@abayer

abayer Jan 31, 2017

Author Member

Aaaah, that's what you meant by DependencyGraph work. I will see how it goes, then.


@Nonnull Set<User> getCulprits();

@Nonnull Calendar getTimestamp();

This comment has been minimized.

Copy link
@jglick

jglick Jan 31, 2017

Member

Why? This is already defined in Run.

This comment has been minimized.

Copy link
@abayer

abayer Jan 31, 2017

Author Member

So I could invoke runWithSCM.getTimestamp() without a bunch of awkward casting - but the mixin approach should help there.

@jglick

This comment has been minimized.

Copy link
Member

commented Jan 31, 2017

And of course you will want a matching PR to workflow-job showing how this will be consumed, and probably one to workflow-scm-step showing how we can take advantage of better APIs to, for example, return a Map<String,String> from the checkout step corresponding to SCM variables.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2017

@jglick Hadn't looked at the DependencyGraph stuff - I'll do so. And yeah, there'll be a matching PR for workflow-job, but I wanted to make sure the approach here was sane before posting that second PR, and also yeah, had been thinking of doing testing in workflow-scm-step anyway.

Switch to a mixin approach
Also bumping to a distinct SNAPSHOT version for dependency builds.
@@ -114,7 +117,9 @@
import java.util.logging.Logger;

import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
import static jenkins.model.Jenkins.*;
import static jenkins.model.Jenkins.checkGoodName;

This comment has been minimized.

Copy link
@abayer

abayer Jan 31, 2017

Author Member

For whatever reason, the wildcard import was barfing for me. In general, I had weirdness with pre-existing imports/scoping here for reasons I'm not clear on.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2017

DependencyGraph work still to come.

abayer added a commit to abayer/workflow-job-plugin that referenced this pull request Jan 31, 2017

[JENKINS-24141] Implement RunWithSCMMixIn.RunWithSCM
Downstream of jenkinsci/jenkins#2730

More work to go, but this is the initial work.

abayer added some commits Apr 25, 2017

Merge branch 'master' into jenkins-24141
Conflicts:
	cli/pom.xml
	core/pom.xml
	pom.xml
	test/pom.xml
	war/pom.xml
Merge branch 'master' into jenkins-24141
Conflicts:
	cli/pom.xml
	core/pom.xml
	pom.xml
	test/pom.xml
	war/pom.xml
@abayer

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2017

Added initial DependencyGraph work - I'm wondering what else needs to be tweaked to just care about AbstractProjects, and am not 100% sure this is a good idea in the first place, but hey.

@jglick jglick removed their assignment May 1, 2017

@jglick jglick self-requested a review May 1, 2017

@reviewbybees

This comment has been minimized.

Copy link

commented May 1, 2017

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.

@abayer

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

@jglick The changes on 7fe3a4a are the ones I'm most wary of - DependencyGraph is so thoroughly tied to an AbstractProject world that I'm just not sure this is a good idea. So your feedback on that in particular would be hugely appreciated.

@abayer

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

Yeah, reverting the DependencyGraph change - if anything, we should tackle that one separately, since it is a pretty broad impact change.

public interface RunWithSCM<JobT extends Job<JobT, RunT> & Queue.Task,
RunT extends Run<JobT, RunT> & RunWithSCM<JobT,RunT> & Queue.Executable> {
@Nonnull
List<ChangeLogSet<? extends ChangeLogSet.Entry>> getChangeSets();

This comment has been minimized.

Copy link
@jglick

jglick May 1, 2017

Member

I was hoping we could take advantage of -source 8 to have all the methods here other than getRunWithSCMMixIn get default implementations, and started prototyping doing this with other mixin classes. Unfortunately it does not seem to work in general (for example for LazyLoadingJob._getRuns()), because apparently a concrete class cannot pick up a default method when the method is defined in an abstract superclass. Thus

public class Demo {
    public static void main(String[] args) {
        System.out.println(new C().answer());
    }
    public static abstract class AC {
        public abstract int x();
    }
    public interface I {
        default int answer() {return 42;}
    }
    public static class C extends AC implements I {}
}

fails to compile:

Demo.java:11: error: C is not abstract and does not override abstract method x() in AC
    public static class C extends AC implements I {}
1 error

However I think it could work here, since the methods you are expecting these Runs to define are not declared in Run. I am still looking for possible uses in ParameterizedJobMixIn & LazyBuildMixIn.

This comment has been minimized.

Copy link
@abayer

abayer May 1, 2017

Author Member

Good times, good times. Lemme know what I should do when you know what I should do. =)

This comment has been minimized.

Copy link
@jglick

jglick May 1, 2017

Member

Well the principle seems to work in #2864, though it is messier there since

  • this was existing code designed to work in -source 7 (or maybe it was even -source 6 at the time, I cannot recall)
  • a lot of the methods were defined in other types

Here I think it should be cleaner. Delete RunWithSCM, asRun, and getRunWithSCMMixIn, and make RunWithSCMMixIn into an interface, with default on as many methods as possible.

This comment has been minimized.

Copy link
@abayer

abayer May 1, 2017

Author Member

Ok, I'll give it a shot.

This comment has been minimized.

Copy link
@abayer

abayer May 1, 2017

Author Member

Hmmm...we're ok with changing RunWithSCMMixIn from an abstract class to an interface? In #2864, you kept the abstract class/interface split between ParameterizedJobMixIn and ParameterizedJob, which seems saner to me. I'm worrying about breaking compatibility for something out there that we may not know about that implements RunWithSCM currently...

This comment has been minimized.

Copy link
@jglick

jglick May 1, 2017

Member

In #2864 I had to maintain that split since it already existed in public APIs. This is fresh code so we can design it more simply.

This comment has been minimized.

Copy link
@abayer

abayer May 2, 2017

Author Member

Ha. I'd forgotten that RunWithSCM was introduced here! d'oh. =)

This comment has been minimized.

Copy link
@abayer

abayer May 2, 2017

Author Member

Ok, so here's what I ended up doing:

  • Renamed RunWithSCMMixIn to RunWithSCM - if it should actually be RunWithSCMMixin, lemme know and I can fix that.
  • Made the non-abstract methods from the old RunWithSCMMixin abstract class into default methods.
  • Got rid of getRunWithSCMMixIn from AbstractBuild.
  • I did keep RunWithSCM.asRun(), however - the getCulprits() and hasParticipants(User) methods both needed to call Run methods. There may very well be a way to do this right without asRun(), and if so, I wanna know what it is. =)
@daniel-beck

This comment has been minimized.

Copy link
Member

commented May 4, 2017

Is this upstream of something?

@jglick

This comment has been minimized.

Copy link
Member

commented May 4, 2017

@daniel-beck see PR description.

@abayer

This comment has been minimized.

Copy link
Member Author

commented May 4, 2017

@daniel-beck @jglick In fairness, I added all those links after his comment. =)

@abayer

This comment has been minimized.

Copy link
Member Author

commented May 5, 2017

@jglick Shall I merge or do you want to?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented May 5, 2017

@abayer I would give @jenkinsci/code-reviewers some time to provide the feedback. 🐝 from me after the quick check. Will merge on Saturday evening if there is no negative feedback.

@oleg-nenashev oleg-nenashev self-assigned this May 5, 2017

@abayer

This comment has been minimized.

Copy link
Member Author

commented May 5, 2017

@oleg-nenashev Alrightie!

public Set<User> calculateCulprits() {
Set<User> c = RunWithSCM.super.calculateCulprits();

AbstractBuild<P,R> p = getPreviousCompletedBuild();

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck May 5, 2017

Member

If this implements caching, where does it consider parallel builds that complete out of order?

This comment has been minimized.

Copy link
@abayer

abayer May 8, 2017

Author Member

shrug? It's the same logic as had previously existed for populating AbstractBuild.culprits, just organized differently.



/**
* RSS feed for changes in this project.

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck May 5, 2017

Member

Should note that @since TODO it was pulled up into job.

This comment has been minimized.

Copy link
@abayer

abayer May 8, 2017

Author Member

Will do.

@oleg-nenashev
Copy link
Member

left a comment

On hold till I move comments from downstream PR by @jglick

@abayer

This comment has been minimized.

Copy link
Member Author

commented May 8, 2017

FYI, I'm doing an ATH run with this to see if anything jumps out.

@abayer

This comment has been minimized.

Copy link
Member Author

commented May 9, 2017

Nothing did jump out from the ATH. @oleg-nenashev are we good to go?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented May 9, 2017

@abayer nope, I have not moved the comments yet though I see several ones are unanswered in the downstream PR

@abayer

This comment has been minimized.

Copy link
Member Author

commented May 9, 2017

@oleg-nenashev Ok, I thought I'd covered all the pertinent ones for this PR.

@oleg-nenashev
Copy link
Member

left a comment

🐝 I confirm that the important comment from the downstream PR has been addressed here.

@reviewbybees done

@abayer abayer merged commit 2f5ca1a into jenkinsci:master May 10, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details
@jglick

This comment has been minimized.

Copy link
Member

commented May 10, 2017

@abayer BTW I think some @since tags did not get updated here.

batmat added a commit to batmat/email-ext-plugin that referenced this pull request Mar 20, 2019

Fix FindBugs violations when building with jenkins.version=2.164
Starting with jenkinsci/jenkins#2730,
`AbstractBuild.getChangeSet()` is `@nonnull`, so we can remove the `if` to
protect against potential NPEs.

Errors were:

```
[INFO] --- spotbugs-maven-plugin:3.1.11:check (default-cli) @ email-ext ---
[INFO] BugInstance size is 4
[INFO] Error size is 0
[INFO] Total bugs: 4
[ERROR] Un appel de méthode dans hudson.plugins.emailext.EmailRecipientUtils.convertRecipientString(String, EnvVars, int, TaskListener) passe null à un paramètre déclaré @nonnull de hudson.model.User.get(String, boolean, Map) [hudson.plugins.emailext.EmailRecipientUtils] At EmailRecipientUtils.java:[line 82] NP_NONNULL_PARAM_VIOLATION
[ERROR] Un appel de méthode dans hudson.plugins.emailext.plugins.recipients.RecipientProviderUtilities.getByLegacyUserCause(Run) passe null à un paramètre déclaré @nonnull de hudson.model.User.get(String, boolean, Map) [hudson.plugins.emailext.plugins.recipients.RecipientProviderUtilities] At RecipientProviderUtilities.java:[line 152] NP_NONNULL_PARAM_VIOLATION
[ERROR] Un appel de méthode dans hudson.plugins.emailext.plugins.recipients.RecipientProviderUtilities.getByUserIdCause(Run) passe null à un paramètre déclaré @nonnull de hudson.model.User.get(String, boolean, Map) [hudson.plugins.emailext.plugins.recipients.RecipientProviderUtilities] At RecipientProviderUtilities.java:[line 133] NP_NONNULL_PARAM_VIOLATION
[ERROR] Test de nullité redondant sur une valeur non nulle dans hudson.plugins.emailext.plugins.recipients.RecipientProviderUtilities.getChangeSetAuthors(Collection, RecipientProviderUtilities$IDebug) [hudson.plugins.emailext.plugins.recipients.RecipientProviderUtilities] Redundant null check at RecipientProviderUtilities.java:[line 75] RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE
[INFO]
```

batmat added a commit to batmat/email-ext-plugin that referenced this pull request Mar 20, 2019

Fix FindBugs violations when building with jenkins.version=2.164
Starting with jenkinsci/jenkins#2730,
`AbstractBuild.getChangeSet()` is `@nonnull`, so we can remove the `if` to
protect against potential NPEs.

Errors were:

```
[INFO] --- spotbugs-maven-plugin:3.1.11:check (default-cli) @ email-ext ---
[INFO] BugInstance size is 4
[INFO] Error size is 0
[INFO] Total bugs: 4
[ERROR] Un appel de méthode dans hudson.plugins.emailext.EmailRecipientUtils.convertRecipientString(String, EnvVars, int, TaskListener) passe null à un paramètre déclaré @nonnull de hudson.model.User.get(String, boolean, Map) [hudson.plugins.emailext.EmailRecipientUtils] At EmailRecipientUtils.java:[line 82] NP_NONNULL_PARAM_VIOLATION
[ERROR] Un appel de méthode dans hudson.plugins.emailext.plugins.recipients.RecipientProviderUtilities.getByLegacyUserCause(Run) passe null à un paramètre déclaré @nonnull de hudson.model.User.get(String, boolean, Map) [hudson.plugins.emailext.plugins.recipients.RecipientProviderUtilities] At RecipientProviderUtilities.java:[line 152] NP_NONNULL_PARAM_VIOLATION
[ERROR] Un appel de méthode dans hudson.plugins.emailext.plugins.recipients.RecipientProviderUtilities.getByUserIdCause(Run) passe null à un paramètre déclaré @nonnull de hudson.model.User.get(String, boolean, Map) [hudson.plugins.emailext.plugins.recipients.RecipientProviderUtilities] At RecipientProviderUtilities.java:[line 133] NP_NONNULL_PARAM_VIOLATION
[ERROR] Test de nullité redondant sur une valeur non nulle dans hudson.plugins.emailext.plugins.recipients.RecipientProviderUtilities.getChangeSetAuthors(Collection, RecipientProviderUtilities$IDebug) [hudson.plugins.emailext.plugins.recipients.RecipientProviderUtilities] Redundant null check at RecipientProviderUtilities.java:[line 75] RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE
[INFO]
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.