Jenkins merge jobs always overwrites PR-URL and Reviewed-By

[orangemocha]

If there is an existing PR-URL or Reviewed-By in a commit message, node-merge-commit/node-accept-pull-request will strip it. This is documented in the wiki though probably not clearly enough:

Note that existing PR-URL: and Reviewed-By: lines in the commit messages will be stripped, and replaced by the ones specified in the form. This only happens when PR-URL or Reviewed-By is found at a beginning of a line. If you want to preserve existing PR-URL: or Reviewed-By: you can simply indent them in your commit messages.

This behavior is not ideal, and it has caused some trouble already. Furthermore, it prevents landing commits with different PR-URL / Reviewed-By metadata in the same job run.

I'd like to come up with a way that also supports the following scenarios:

1. Preserve existing PR-URL / Reviewed-By and don't add any such fields
2. In case of backports / forwardports, preserve original metadata but also add new metadata. Indenting the old metadata with a space or > could help distinguish the two.

[orangemocha]

I wanted to make sure that @nodejs/collaborators are aware of this limitation.

[trevnorris]

Preserve existing PR-URL / Reviewed-By and don't add any such fields

I think this is the only viable solution if it is meant to be used to cherry-pick commits from master onto stable release branches. Those both already have metadata and we'll probably be cherry-picking many different commits onto a single branch/PR.

[trevnorris]

Added tsc-agenda to discuss whether we're planning on using Jenkins to land patches on major release branches.

[orangemocha]

The simplest approach seems to be to never remove anything that's already in the commit messages, and only add metadata from the form when non-empty values are entered/selected.

[trevnorris]

Yeah. That would be perfect. Though I don't want to be the solo voice here, so want to briefly discuss it in the TSC meeting.

@orangemocha How does a new release branch get into Jenkins? e.g. when v5 is cut will that branch need to be manually added for rebase?

[orangemocha]

It's a very simple change: deleting a line from a script. I guess I was worried about duplicate information, but there's definitely less harm in duplicate metadata than deleted metadata.

No extra steps are required for handling future branches. PRs against them will work, and you can specify any branch to node-merge-commit. The initial creation of the branch will have to be done manually. I hope I understood your question...

It should be noted that the changes to support flaky tests are not in the v3.x branch at the moment.

[orangemocha]

I won't be attending the TSC meeting today. Please let me know what comes out of that discussion. It should be pretty safe to make the change.

[thefourtheye]

@orangemocha How about having a checkbox which says "Remove metadata"?

[cjihrig]

I don't think we should ever allow Jenkins to remove metadata.

[orangemocha]

How about having a checkbox which says "Remove metadata"?

That's also easy to do. It would be unchecked by default.

[orangemocha]

Ok, to move forward with the proposed change? That is, to make Jenkins only add metadata when specified, and never remove it? I tried to parse the contents of the TSC meeting notes but I would prefer a +1 by someone who was actually there :)

[jasnell]

+1
On Sep 2, 2015 6:18 PM, "Alexis Campailla" notifications@github.com wrote:

Ok, to move forward with the proposed change? That is, to make Jenkins
only add metadata when specified, and never remove it? I tried to parse the
contents of the TSC meeting notes but I would prefer a +1 by someone who
was actually there :)

—
Reply to this email directly or view it on GitHub
#179 (comment).

[orangemocha]

Made the change and updated the wiki. @nodejs/collaborators : the automated merge jobs have been changed to never remove any existing metadata lines (PR-URL:, Reviewed-By:, etc) from commit messages. Anything you specify in the form, if not empty, it's simply added at the end of the commit message.

Note that node-accept-pull-request always adds the current PR-URL. I don't think that this will conflict with the scenario of porting / cherry-picking multiple commits. If it's done with a PR, then the additional PR-URL will be added.

[trevnorris]

Note that node-accept-pull-request always adds the current PR-URL.

@rvagg Are we adding the additional PR-URL to the cherry picked commits for release branches?

[trevnorris]

Or, I might be thinking of the wrong Jenkins job.

[rvagg]

This is a policy that will adapt over time but currently I think we're at: if the cherry-pick is clean or easy then it can go straight in, where it requires a lot of massaging and you're not the primary and/or you need review then it should be done via PR. In the case of a PR cherry-pick I'd like to have the two sets of metadata with the original indented. i.e. I'd like us to preserve existing metadata (PR-URL, Reviewed-By, Fixes, Closes) to be indented and any new metadata to be appended to the end. This is also interesting for the tooling we use for releases. We use https://github.com/rvagg/commit-stream to parse this data in both https://github.com/rvagg/changelog-maker which produces changelogs for us and https://github.com/rvagg/iojs-tools/tree/master/branch-diff which I use to determine what needs to be cherry-picked. So we require predictable metadata formats so our tooling works properly into the future.

[trevnorris]

So regardless, there is a path where we need to be able to land commits with existing metadata, and without a PR-URL appended. /cc @orangemocha

@rvagg Can we say that regardless of whether the cherry-pick landed in a PR, if it applies cleanly then it doesn't need the additional metadata? Also, are you implying we can land clean cherry-picks w/o opening a PR? If this were the case, and the PR is labeled for landing on the stable branch, then it would be much easier for developers to move commits over to stable.

I'm trying to think of ways we can make the cherry-picking easier for everyone to participate. My original plan, when I proposed it, was that PRs would have tags like cherry-pick-stable and cherry-pick-LTS. If the PR was labeled with on of these then the committer could land it immediately on the given branch if it was clean. Or if not then another PR could be opened, and a branch made on nodejs/node with a predictable pattern like refs/cherry-picks/pr42 where the patch would be migrated. Then before release we could check the outstanding issues.

These steps are straight forward and easy enough for any collaborator to follow. If it got to the point that resolution of the conflict was hard then they could hand over the work in the PR they opened. But at least it would be there to work on. I'm finding it difficult to help out with the cherry-picking PRs today because they have gotten so massive. And I can't see at first glance which had conflicts while being picked.

[rvagg]

I've been pondering similar thoughts but where I come to is that we're too early to be heavy on process here, at the moment our v3.x and v4.x branches are really immature and it's a simple process. As we get further along on the the 6+ months of a stable branch it's going to get increasingly complex and frustrating and we'll need to figure out process by then, but until we have a feel for the kinds of difficulty we're optimising around I'd rather not assume too much right now. Currently the cherry-pick-stable tag idea isn't very useful because it applies to almost anything. In fact, I made a land-on-v3.x for this purpose but only used it once. I think it'll be more useful when a branch goes LTS because we'll need to be a lot more choosy wrt what lands. So I'm thinking that, at least initially, those labels won't be helpful for stable branches, they may be later in a branch's life, but they will certainly be helpful for LTS branches.

Also, I've been pondering how we can build better tooling for this, using GitHub APIs. Right now I've started with https://github.com/rvagg/iojs-tools/tree/master/branch-diff which works like git log but only uses summary + PR-URL to decide what's changed, so it's very accurate for our specific use. I was thinking that coupling that data with a web dashboard that shows us what could be done and helps us to tag and identify what should be done and then we can check against when we release that what should have been done was done.

But again, it's early, and we shouldn't kid ourselves that we have a full grasp on the future, so I'm happy to let this one play out.

In the meantime I'd be happy letting people cherry-pick their own stuff to stable as long as there's nothing breaking and the cherry-pick resolution isn't too complex. I'm also happy having only a few of us doing it and then deferring the hard ones to PRs for reviews, like I did here: nodejs/node#2608

[trevnorris]

Currently the cherry-pick-stable tag idea isn't very useful because it applies to almost anything.

Should have clarified, that was just a placeholder label for the label I just realized we had: land-on-4x. Now that I know about it, I'm using it to label PRs that should make it to v4 at some point, but they aren't necessary for the 4.0.0 release. I assume that's what it's there for? Since ATM there's only a 4.0.0 milestone, and I don't want to forget about PRs I know should eventually land on 4.x.

In the meantime I'd be happy letting people cherry-pick their own stuff to stable as long as there's nothing breaking and the cherry-pick resolution isn't too complex.

Awesome. If a PR from master lands cleanly on 4x I'll be happy to flip through and get those in. Though that is conditional on the CI allowing me to push them over without it adding PR-URL info. And for those that don't apply cleanly, and after 4.0.0 is released, it'll be easy to open PRs for those that do conflict. If this process is good then it shouldn't be hard to flip through everything a couple times a week.

[orangemocha]

If you use node-merge-commit you don't need to set the PR-URL. node-accept-pull-request only works on PRs, it uses node-merge-commit internally and it passes the PR-URL to it. So if you port over the cherry-pick with a PR, then there will that PR URL added. Seems reasonable. If you do it with a temporary branch and by using node-merge-commit, then there will be nothing added.

[orangemocha]

You can also land the cherry picks in a PR without adding the PR-URL, by using node-merge-commit and specifying refs/pull/PR_ID/head as the commit.

[trevnorris]

@orangemocha Thanks.