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

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

Closed
cgsheeh opened this issue Feb 2, 2024 · 10 comments

Comments

@cgsheeh
Copy link
Member

cgsheeh commented Feb 2, 2024

See #2004 (comment) 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
Copy link
Collaborator

marco-c commented Feb 2, 2024

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

@cgsheeh
Copy link
Member Author

cgsheeh commented Feb 2, 2024

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
Copy link
Collaborator

marco-c commented Feb 5, 2024

@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 mozilla/libmozevent#104 for that).

@cgsheeh
Copy link
Member Author

cgsheeh commented Feb 5, 2024

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
Copy link
Collaborator

marco-c commented Feb 5, 2024

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
Copy link
Member Author

cgsheeh commented Feb 5, 2024

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 marco-c changed the title apply uplift stacks against the latest commits from the destination repo Always apply uplift stacks against the latest commits from the destination repo for release, esr, beta Feb 5, 2024
@marco-c
Copy link
Collaborator

marco-c commented Feb 5, 2024

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
Copy link
Member Author

cgsheeh commented Feb 5, 2024

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

@marco-c
Copy link
Collaborator

marco-c commented Feb 5, 2024

@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
Copy link
Collaborator

marco-c commented Mar 7, 2024

Fixed by #2094 (mozilla/libmozevent#105), released in 084750a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants