search

mozilla / code-review

Automated static analysis & linting bot for Mozilla repositories
Mozilla Public License 2.0
55 stars 38 forks source link

Always apply uplift stacks against the latest commits from the destination repo for release, esr, beta #2057

Closed cgsheeh closed 4 months ago

cgsheeh commented 5 months ago

See https://github.com/mozilla/code-review/issues/2004#issuecomment-1921876726 for details. When we apply uplift patches in code-review-bot, we should apply them against the latest commits from the target repo.

marco-c commented 5 months ago

@cgsheeh is refs.base.identifier correct in general or should we switch to attachments.metrics.recentCommit.identifier?

cgsheeh commented 5 months ago

attachments.metrics.recentCommit.identifier from the diffusion.repository.search endpoint would give a recent commit in the target repo. As discussed on Slack, just falling back to tip when using the canonical uplift repo (ie mozilla-beta instead of mozilla-unified) would give the same result as well.

marco-c commented 5 months ago

@cgsheeh sorry to ask again, it wasn't clear to me: is refs.base.identifier not the same as attachments.metrics.recentCommit.identifier? In other terms, should we switch from refs.base.identifier to attachments.metrics.recentCommit.identifier?

Anyway, we are already applying on "tip" now, probably the failures are just because we are not as good as Lando at applying patches (I filed https://github.com/mozilla/libmozevent/issues/104 for that).

cgsheeh commented 5 months ago

I'm pretty sure they are not the same. attachments.metrics.recentCommit.identifier is a field on the diffusion.repository.search response when the metrics attachment is requested, and should be the most recent commit in the repo each time that API endpoint is requested.

refs.base.identifier is a field on the differential.diff.search response that is the base identifier for the diff when it is created. It is just a reference to the base commit where the diff should be able to apply.

Yes, if we can just apply to tip, we shouldn't need either of these identifiers IMO. :)

marco-c commented 5 months ago

OK, thanks for the clarification! So we can keep using refs.base.identifier and we don't have to switch to attachments.metrics.recentCommit.identifier.

Now, last clarification, on beta, release, and esr, should we ignore "refs.base.identifier" and always apply to "tip"? If yes, then we need to introduce some logic to do that in libmozevent. If not, then we can close this as it's already happening.

cgsheeh commented 5 months ago

Now, last clarification, on beta, release, and esr, should we ignore "refs.base.identifier" and always apply to "tip"? If yes, then we need to introduce some logic to do that in libmozevent. If not, then we can close this as it's already happening.

Yes, that's what we should be doing. Lando will be applying the patch to the tip, so we want code-review-bot to apply it there as well and give early feedback about if the patch will apply successfully or not (along with all the lint/style checks). :)

marco-c commented 5 months ago

All right, this will require a change in libmozevent: basically add a new config at https://github.com/mozilla/libmozevent/blob/master/libmozevent/mercurial.py#L61 so that we can specify if a repo must use the latest revision or not, and change the logic around base revision at https://github.com/mozilla/libmozevent/blob/ddb136df436b589a0762447c455eb380a2cec7df/libmozevent/mercurial.py#L137.

cgsheeh commented 5 months ago

@marco-c are you able to take this on or should I have a go at it?

marco-c commented 5 months ago

@cgsheeh it'd be great if you can, and then I can do the steps to update libmozevent in code-review and release a new version of code-review.

marco-c commented 4 months ago

Fixed by https://github.com/mozilla/code-review/pull/2094 (https://github.com/mozilla/libmozevent/pull/105), released in https://github.com/mozilla/code-review/commit/084750a1ba5896db9a9c5787f857ad1b4683e1aa.