[JENKINS-45864] Add SCM instance attribute to ChangeLogSet #2955

thatrevguy · 2017-07-31T20:33:38Z

See JENKINS-45864.

@jenkinsci/code-reviewers

daniel-beck · 2017-07-31T20:38:23Z

@abayer Didn't you recently build something related?

oleg-nenashev

The change looks good to me, my comments are mostly about better diagnosability and static analysis

oleg-nenashev · 2017-08-02T09:46:20Z

core/src/main/java/hudson/scm/ChangeLogParser.java

@@ -42,6 +42,17 @@
 public abstract class ChangeLogParser {

    /**
+     * @since


@since TODO is better

oleg-nenashev · 2017-08-02T09:47:40Z

core/src/main/java/hudson/scm/ChangeLogParser.java

+        if (build instanceof AbstractBuild && Util.isOverridden(ChangeLogParser.class, getClass(), "parse", AbstractBuild.class, SCM.class, File.class)) {
+            return parse((AbstractBuild) build, scm, changelogFile);
+        } else {
+            throw new AbstractMethodError("You must override the newer overload of parse");


"... to support non-Pipeline jobs".

Ideally we need a @RequireOverride annotation to make these cases explicit during the compilation/static analysis

oleg-nenashev · 2017-08-02T09:48:10Z

core/src/main/java/hudson/scm/ChangeLogParser.java

@@ -53,6 +64,11 @@
    }

    @Deprecated
+    public ChangeLogSet<? extends Entry> parse(AbstractBuild build, SCM scm, File changelogFile) throws IOException, SAXException {


Makes sense to add Javadoc and to document why

oleg-nenashev · 2017-08-02T09:48:45Z

core/src/main/java/hudson/scm/ChangeLogSet.java

@@ -61,6 +61,17 @@
    @Deprecated
    public final AbstractBuild<?,?> build;
    private final RepositoryBrowser</* ideally T */?> browser;
+    private final SCM scm;


Add @CheckForNull annotation

oleg-nenashev · 2017-08-02T09:49:19Z

core/src/main/java/hudson/scm/ChangeLogSet.java

@@ -69,6 +80,7 @@ protected ChangeLogSet(Run<?,?> run, RepositoryBrowser<?> browser) {
        this.run = run;
        build = run instanceof AbstractBuild ? (AbstractBuild) run : null;
        this.browser = browser;
+        this.scm = null;


Use this(run, browser, null) to avoid code duplication

oleg-nenashev · 2017-08-02T09:49:31Z

core/src/main/java/hudson/scm/ChangeLogSet.java

+     * Repository SCM of this change set.
+     * @return a SCM instance or null.
+     */
+    public SCM getSCM() { return scm; }


Add @CheckForNull

oleg-nenashev · 2017-08-02T09:50:14Z

core/src/main/java/hudson/scm/ChangeLogSet.java

+    /**
+     * @since
+     */
+    protected ChangeLogSet(Run<?,?> run, RepositoryBrowser<?> browser, SCM scm) {


Nice2have, document parameters, add argument annotations

oleg-nenashev · 2017-08-02T17:59:51Z

core/src/main/java/hudson/scm/ChangeLogParser.java

@@ -52,6 +63,16 @@
        }
    }

+    /**
+     * Method for compatibility with AbstractBuild.


I would just say, @deprecated this method should not be used directly anymore, use the {@link ...} method instead. Should not override annotation may be helpful as well (@Restricted(ProtectedExternally.class) )

A little confused on how AbstractBuild will behave when it runs this.

If a SCM plugin only overrides the Run based parse function will AbstractBuild run it or still revert to AbstractBuild based parse function?

Guessing it would go for the latter function. Which would require an override to populate SCM attribute on ChangeLogSet produced from AbstractBuild. Otherwise we are stuck in a catch 22 situation.

Preferably, the AbstractBuild based parse function would execute:
return parse((Run) build, scm, changelogFile);

But no SCM plugins implement this yet.

This is a good point. Maybe it makes sense to name the new method as parseRun() to prevent ambiguity. Then all internal calls may be safely moved to the new method

daniel-beck · 2017-08-04T11:46:23Z

core/src/main/java/hudson/scm/ChangeLogParser.java

+     * @since TODO
+     */
+    public ChangeLogSet<? extends Entry> parseRun(Run build, SCM scm, File changelogFile) throws IOException, SAXException {
+        if (build instanceof AbstractBuild && Util.isOverridden(ChangeLogParser.class, getClass(), "parse", AbstractBuild.class, SCM.class, File.class)) {


Method renamed, also AME below needs adapting.

oleg-nenashev · 2017-08-04T18:52:35Z

core/src/main/java/hudson/scm/ChangeLogParser.java

+     * ChangeLogSet SCM attribute in AbstractBuild projects.
+     */
+    @Deprecated
+    public ChangeLogSet<? extends Entry> parseRun(AbstractBuild build, SCM scm, File changelogFile) throws IOException, SAXException {


I would rather keep it as just "parse()" since it is not Run. Renaming both methods didn't help to avoid ambiguity I was referencing before

oleg-nenashev · 2017-08-04T18:53:21Z

Would be nice to get it in the weekly so it would have a chance to get into the next LTS baseline

daniel-beck · 2017-08-05T05:21:34Z

Would prefer this gets a proper review to ensure the additions are sane.

oleg-nenashev · 2017-08-05T14:47:44Z

@daniel-beck put it on hold

oleg-nenashev · 2017-08-12T22:59:21Z

@daniel-beck seems it's going to miss the weekly. Would be great if you find some time to review it before the next one

thatrevguy · 2017-08-29T08:41:05Z

Hello, anything else needed from me at this point?

daniel-beck · 2017-08-29T16:17:23Z

@hathoward No, thanks for checking in. Jenkins World has been sucking up our time the last few weeks, so this has been delayed.

oleg-nenashev

The updated version LGTM, needs re-review by @daniel-beck

daniel-beck · 2017-09-13T06:24:57Z

Would be more comfortable if @jglick could take a look at this.

jglick

Does not seem like a good idea. Adding a new persistent field to ChangeLogSet may or may not be desirable (would need to consider the XStream-serialized form). At least, on its own it does not address the stated goal

a reliable way to identify correct remote URL for a ChangeLogSet instance

which based on the references to the git plugin I presume refers to Git changelogs. It is also unclear from JIRA what kind of report is needed—in the UI? Via Stapler exported …/api/json? Solely via Java API?

In any event there is no apparent downstream PR showing how this would be used, by git-plugin, or the reporter’s

plugin for handling deployment COs

Certainly it would need at a minimum some kind of new call for Pipeline, thus a PR using timestamped snapshots in workflow-job-plugin, and some sort of functional test demonstrating the newly available information being used successfully.

It is not even obvious to me that any core API change is needed here at all. There is nothing preventing an implementation of SCM.createChangeLogParser from retaining some configuration fields from the SCM, such as a “remote URL”. The fact that GitSCM does not do so currently is presumably because no one cared before.

Whether any newly added getter in GitChangeSetList or GitChangeSet (unclear which) would need to @Override some API method in core would depend on whether there is a meaningful common definition of “remote URL”. Subversion, for example, uses URLs for everything, but the “repository” is just a server base URL, whereas projects typically operate on some subdirectory; and its changelog entries are able to specify both relative and absolute paths.

jglick · 2017-09-18T20:53:38Z

Adding a new persistent field to ChangeLogSet may or may not be desirable

Ignore, it is not serializable.

thatrevguy · 2017-09-20T22:14:08Z

Thank you for taking a look at this.

It is also unclear from JIRA what kind of report is needed—in the UI? Via Stapler exported …/api/json? Solely via Java API?

We have a plugin that generates and manages change orders in Service Now. Report is change order itself with contents including information about the change sets that triggered CICD pipeline. Reliably generating working URLs from change sets has been difficult when a pipeline performs multiple checkouts.

In any event there is no apparent downstream PR showing how this would be used, by git-plugin

I have not figured out how to build a plugin against a non upstream Jenkins core build. If there is a trivial way of doing that it'd be much appreciated to learn.

It is not even obvious to me that any core API change is needed here at all. There is nothing preventing an implementation of SCM.createChangeLogParser from retaining some configuration fields from the SCM, such as a “remote URL”. The fact that GitSCM does not do so currently is presumably because no one cared before.

This looks very much doable. I'll investigate and close this PR if I get a working PR for git-plugin.

daniel-beck · 2017-09-21T00:19:08Z

@hathoward

I have not figured out how to build a plugin against a non upstream Jenkins core build. If there is a trivial way of doing that it'd be much appreciated to learn.

Any registered user (accounts.jenkins.io) can upload snapshot builds of anything to repo.jenkins-ci.org (mvn deploy). These will get a unique timestamp on upload for unambiguity. You can specify that version as jenkins.version property in plugin POMs.

batmat · 2019-03-19T10:01:15Z

Hello @thatrevguy! do you plan to get back address comments here?

I am going to propose this PR for closing, and will close it if no action is taken in the next days or so.
Please feel free to comment anytime, even after it would be closed, so we reopen if you'd like to work on finishing it.

Thanks!

Add SCM instance attribute to ChangeLogSet

c157a01

daniel-beck changed the title ~~Add SCM instance attribute to ChangeLogSet~~ [JENKINS-45864] Add SCM instance attribute to ChangeLogSet Jul 31, 2017

Fix compatibility issues with AbstractBuild

298123c

oleg-nenashev reviewed Aug 2, 2017

View reviewed changes

Add diagnosability and static analysis suggestions

13f2516

oleg-nenashev reviewed Aug 2, 2017

View reviewed changes

Switch new logparser methods to parseRun

1024cb1

daniel-beck reviewed Aug 4, 2017

View reviewed changes

thoward-godaddy added 2 commits August 4, 2017 09:12

Update method name arg for isOverridden call

c386c1a

Update method name in exception message

d1250ec

oleg-nenashev reviewed Aug 4, 2017

View reviewed changes

oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label Aug 4, 2017

thoward-godaddy added 2 commits August 4, 2017 12:30

Revert method name change for AbstractBuild parse overload

15a009c

Update AbstractBuild to use reverted method name.

aa6cffb

oleg-nenashev approved these changes Aug 4, 2017

View reviewed changes

oleg-nenashev added the on-hold This pull request depends on another event/release, and it cannot be merged right now label Aug 5, 2017

daniel-beck self-assigned this Aug 12, 2017

oleg-nenashev approved these changes Sep 2, 2017

View reviewed changes

daniel-beck requested a review from jglick September 13, 2017 06:24

jglick requested changes Sep 15, 2017

View reviewed changes

daniel-beck removed their assignment Jan 22, 2018

batmat added proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it stalled The PR is reasonable and might be merged, but it is no longer active. It can be taken over by other labels Mar 19, 2019

daniel-beck closed this Mar 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-45864] Add SCM instance attribute to ChangeLogSet #2955

[JENKINS-45864] Add SCM instance attribute to ChangeLogSet #2955

thatrevguy commented Jul 31, 2017 •

edited by daniel-beck

Loading

daniel-beck commented Jul 31, 2017

oleg-nenashev left a comment

oleg-nenashev Aug 2, 2017

oleg-nenashev Aug 2, 2017

oleg-nenashev Aug 2, 2017

oleg-nenashev Aug 2, 2017

oleg-nenashev Aug 2, 2017

oleg-nenashev Aug 2, 2017

oleg-nenashev Aug 2, 2017

oleg-nenashev Aug 2, 2017

thatrevguy Aug 2, 2017 •

edited

Loading

oleg-nenashev Aug 3, 2017

daniel-beck Aug 4, 2017

oleg-nenashev Aug 4, 2017

oleg-nenashev commented Aug 4, 2017

daniel-beck commented Aug 5, 2017

oleg-nenashev commented Aug 5, 2017

oleg-nenashev commented Aug 12, 2017

thatrevguy commented Aug 29, 2017

daniel-beck commented Aug 29, 2017

oleg-nenashev left a comment

daniel-beck commented Sep 13, 2017

jglick left a comment

jglick commented Sep 18, 2017

thatrevguy commented Sep 20, 2017

daniel-beck commented Sep 21, 2017

batmat commented Mar 19, 2019

[JENKINS-45864] Add SCM instance attribute to ChangeLogSet #2955

[JENKINS-45864] Add SCM instance attribute to ChangeLogSet #2955

Conversation

thatrevguy commented Jul 31, 2017 • edited by daniel-beck Loading

daniel-beck commented Jul 31, 2017

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thatrevguy Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented Aug 4, 2017

daniel-beck commented Aug 5, 2017

oleg-nenashev commented Aug 5, 2017

oleg-nenashev commented Aug 12, 2017

thatrevguy commented Aug 29, 2017

daniel-beck commented Aug 29, 2017

oleg-nenashev left a comment

Choose a reason for hiding this comment

daniel-beck commented Sep 13, 2017

jglick left a comment

Choose a reason for hiding this comment

jglick commented Sep 18, 2017

thatrevguy commented Sep 20, 2017

daniel-beck commented Sep 21, 2017

batmat commented Mar 19, 2019

thatrevguy commented Jul 31, 2017 •

edited by daniel-beck

Loading

thatrevguy Aug 2, 2017 •

edited

Loading