Merge 17.8 beta 2 into develop

[mokagio]

Includes:

• Fix for an issue introduced in 17.3 to only extract author info on a new Post / Page if the user can query for authors (Only extract author info on a new Post / Page if the user can query for authors. #16870) by @twstokes
• Fix comment moderation options on Atomic sites (Comments: fix moderation options on Atomic sites #16869) by @ScoutHarris
• Fix enable stats module flow (Stats: Fix enable stats module flow #16873 and Can't enter Jetpack Install / Connect flow from Stats #16867) by @momo-ozawa

Nothing to test, if CI Shows green, we're good.

Regression Notes

1. Potential unintended areas of impact
   N.A.

2. What I did to test those areas of impact (or what existing automated tests I relied on)
   N.A.

3. What automated tests I added (or what prevented me from doing so)
   N.A.

---

• I have completed the Regression Notes.
• I have considered adding unit tests for my changes. N.A.
• I have considered adding accessibility improvements for my changes. N.A.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary. N.A.

[peril-wordpress-mobile]

You can trigger an installable build for these changes by visiting CircleCI here.

[peril-wordpress-mobile]

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

[peril-wordpress-mobile]

	Messages
📖	This PR has the 'Releases' label: some checks will be skipped.

Generated by 🚫 dangerJS

[oguzkocer]

@mokagio I am confused about the commit message for 699c48f. Previous commit 647a60a is equal to the current release/17.8 which is what I'd expect. However, I'd then expect a merge from origin/develop into the branch whereas the commit message is Merge branch 'release/17.8' into merge/release-17.8-beta-2-into-develop.

I tried creating a branch from release/17.8 and merged origin/develop into it and that branch is equal to this one, so I am approving the PR. However, it might be worth closing this PR and opening one with the correct message. (assuming it's incorrect in the first place)

Also, it looks like release/17.8 can be cleanly merged into develop, so I don't think the extra commit is even necessary.

[AliSoftware]

List of commits and changes match the list of PRs included in this beta as per the PR description, so 👍

I'm a bit surprised to see the release notes source have been updated for WP but I don't see a jetpack release notes equivalent in this diff… was this missed? On the other end I don't see a default nor en-US folder in jetpack_metadata either 🤔 maybe you have implemented some kind of auto-fallback to WP implemented in tooling for cases like if they are the same for both hence only updating the WP metadata here?

Approving anyway because this is not a blocker and release notes will be downloaded from GP again later on finalize_release anyway, but just wanted to raise this in advance to be sure.

[AliSoftware]

However, I'd then expect a merge from origin/develop into the branch whereas the commit message is Merge branch 'release/17.8' into merge/release-17.8-beta-2-into-develop.

@oguzkocer I might be wrong but when I saw this commit I interpreted it as "Gio created the intermediate branch but then added new commits to the release branch later like the metadata stuff, so he merged release/17.8 into the intermediate branch again to make it include those too"; which honestly could have just been a fast-forward if that's the case rather than a merge commit, but as long as it contains the expected commits and the end result is the same, is ok too 🙂

(Which means I agree with you that the intermediate branch was probably not strictly necessary)

But maybe I misread that commit and you're right in your suspicion, so worth waiting for Gio to validate before merging 🙃

[oguzkocer]

@oguzkocer I might be wrong but when I saw this commit I interpreted it as "Gio created the intermediate branch but then added new commits to the release branch later like the metadata stuff, so he merged release/17.8 into the intermediate branch again to make it include those too"; which honestly could have just been a fast-forward if that's the case rather than a merge commit, but as long as it contains the expected commits and the end result is the same, is ok too 🙂

@AliSoftware I thought about that too, but then I checked the commits and 647a60a is clearly equal to release/17.8 as shown below. And the reason I was checking this is, I don't think we want to create our merge branches from develop and merge the release branch into it, because it doesn't communicate our intention correctly.

[AliSoftware]

Ah, good catch!

Yeah I agree, I prefer doing it the other way around too (= cut intermediate from release then merge develop in it rather than the opposite) like you suggest, exactly for that reason; even if the end result ends up the same, it conveys our intent better indeed 👍

[mokagio]

@oguzkocer @AliSoftware: thank you for your reviews. I've seen your comments, I'm at my EOD and EOW and don't have time to give them thoughtful reply they deserve.

I'm going to merge this now (because you both approved it) to integrate the bug fixes and avoid conflicts against develop as it keeps evolving.

[mokagio]

Also, I'm not sure about the Jetpack failure in CI on that commit but:

• Jetpack builds locally for me
• The build reached TestFlight alright, which is another data point about it building fine

[AliSoftware]

Got the same Jetpack build failure on CI on this other PR though 🤔 so maybe not just a fluke?

[mokagio]

I'm a bit surprised to see the release notes source have been updated for WP but I don't see a jetpack release notes equivalent in this diff… was this missed? On the other end I don't see a default nor en-US folder in jetpack_metadata either 🤔 maybe you have implemented some kind of auto-fallback to WP implemented in tooling for cases like if they are the same for both hence only updating the WP metadata here?

Sharp as usual @AliSoftware. That's actually a bug in my setup. During the 17.7 submission, I noticed that the English (US) "What's new" field was empty for Jetpack, but I'm still to address that.

[mokagio]

@oguzkocer @AliSoftware: I both created the intermediate branch and did if from develop intentionally, not as a byproduct of other actions during the beta work.

My rationale for using intermediate branch is that, while there were no conflicts at the time of opening the PR between develop and release/17.8, nothing guarantees they would not arise as develop evolved while the PR sat waiting for review. Having the intermediate branch already made sure I could resolve those conflicts without having to create a new PR for it.

I'm curious to understand why the branching approach I use doesn't communicate the intent clearly? Where I'm coming from is that we want the changes from the release branch to go into develop, so merging develop into it, or rather the intermediate branch, seems to me like we're moving changes into the "wrong" branch. 🤔

To be clear: I'm not opposed to either of the other options: doing a straightforward develop <- release/* when possible (which is cleaner) and accept the "risk" of possibly having to create an intermediate branch if conflicts arise; and merging develop into the intermediate branch, then the intermediate branch into develop. I also don't think they're wrong, but –with my current understanding– I prefer the approach I've taken here.

Keen to hear from you 😊 Thanks. (I'll be biting my nails worried I missed some Git 101 all this time till I hear back 🤣)

[oguzkocer]

@mokagio Think of it this way, if you wanted to merge branchA into develop and if there was a merge conflict, what would you do? You'd merge develop into your branch and resolve the conflict. Or, if we preferred rebase (which we typically only do before a PR is opened) then you'd rebase branchA on top of develop so the merge conflict would be resolved.

This scenario is not any different - the only reason we want to create the intermediate branch is because we don't want to merge develop into the release branch, so we create a new branch identical to the release branch and then merge develop into it. And when I say this communicates your intention better, I mean that your intention was to resolve the conflict with develop, but you couldn't do it within the release branch, so you branch off from it.

I don't think I've ever create a branch from develop and immediately merged a branch into it 🤔. The only scenario I can think of I have done anything similar is, if I branch from develop to work on something and then I realize I need changes from another branch, so I merge that in. Even then, I'd probably do either a rebase or a cherry-pick to have a cleaner history.

Does that make sense?

[oguzkocer]

@mokagio Here is one more thing to consider: When you start from the base branch and merge the release branch into it, you create 96f3f4b which is basically the whole PR because that's what applied all the changes. Even the commit list in the PR is a bit misleading, because the commits that show up are actually from the merge commit and they were not directly applied.

Here is the same merge commit when I start from the release branch and merge trunk into it a7a8073. The only changes that show up there are the ones that were merged from trunk, and I think the only reason that even shows up is because of git is not able to tell those are the same files. (which we should look into)

[mokagio]

Thanks @oguzkocer. Your explanation makes sense to me.

@mokagio Think of it this way, if you wanted to merge branchA into develop and if there was a merge conflict, what would you do? You'd merge develop into your branch and resolve the conflict. Or, if we preferred rebase (which we typically only do before a PR is opened) then you'd rebase branchA on top of develop so the merge conflict would be resolved.

I always rebase when I can. I wish there was a way from GitHub to resolve the conflicts without having to pull the base branch into the PR branch. The only way to achieve it achieve it woulb be doing the merge locally and pushing it, but that's obviously not an option in a large team.

Even the commit list in the PR is a bit misleading, because the commits that show up are actually from the merge commit and they were not directly applied.

Can you elaborate on that? If I understand correctly, your point is that the commit that goes into the base branch is the merge commit, so that's the only commit that should appear in the PR commit list? Edit: I thin I got it, see comment below.

I find it useful to see that commit list, in particular the merge commits for the PRs that went into the release branch. It helps me understand what went into that beta PR.

Thank you for taking the time 😄 🙇

[mokagio]

@oguzkocer

Can you elaborate on [the commit list]?

Nevermind that. I used your WordPress-Android 17.8 beta 3 PR as an experiment to check this opening a PR with the branching strategy I used here.

I can see what you meant regarding the commits. In that PR, your merge commit of develop into the intermediate branch cut off the release one shows the result of the conflict resolution. In my counter PR where the merge is of the release branch into the intermediate branch cut off develop all of the changes are show. That's exactly what you said in your comment.

While I like the fact that the merge commit shows all the changes that went in with it in merge/release-x-into-develop <- release/x, the diff for the other strategy is much more informative because it shows how conflicts were resolved.

Thanks again. I think I'll have to P2 / Field Guide about this soon to make sure 1) I assimilate it, 2) future devs (and forgetful me) will have a documented rationale for this approach.

[oguzkocer]

Even the commit list in the PR is a bit misleading, because the commits that show up are actually from the merge commit and they were not directly applied.

@mokagio I see that we are mostly on the same page now. Just wanted to clarify one thing. The misleading part about the commit list is that in that scenario, we'd actually have one merge commit which brings in a bunch of other commits. The fact that Github lists them is a good thing, it just can be interpreted as if those commits are applied to the branch directly which is not the case when we start from develop/trunk.

Basically, starting from release branch and develop/trunk will list the same commits and while the former would have commits applied directly and the latter would bring the commits from the merge. Hope that clarifies the last point!

While I like the fact that the merge commit shows all the changes that went in with it in merge/release-x-into-develop <- release/x, the diff for the other strategy is much more informative because it shows how conflicts were resolved.

Exactly! Not doing a merge commit is even better (if possible), because it shows there were no conflicts which makes the PR easier to review.