-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix mirroring from upstream to downstream with ranges #24155
Conversation
This PR needs to be reviewed with the approach proposed in #23856. |
Ah, I forgot there was an existing PR that also did this, or at least something similar. Since Philip is now away and won't be back for a long while, I think that we should close that PR and combine its changes into this one. I say this because I'd request changes on some of the code for both bug fixes and improved logic. One key difference between that PR and this one is that it bases whether to add the range delimiter if the downstream version is the first release, but this PR only adds the range delimiter if the downstream version is the first release of the matching engine version, which means that less ranges are introduced during mirroring. (In Philip's PR, Chrome 23 would still become Edge ≤79 during mirroring.) |
This pull request has merge conflicts that must be resolved before it can be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just three comments to make the implementation a bit clearer. And could we add a unit test?
Co-authored-by: Claas Augner <[email protected]>
Co-authored-by: Claas Augner <[email protected]>
Co-authored-by: Claas Augner <[email protected]>
This pull request has merge conflicts that must be resolved before it can be merged. |
Since the changes requested have been applied, I'm going to go ahead and merge this since an LGTM was given! |
This PR fixes the mirroring script when ranges are involved. Apparently, if the upstream release was a range, the downstream browser would not be a range, which meant that various bits of data was not set to mirror when it should. This PR fixes this by introducing some logic that checks if the previous release (before the matching release) uses the same engine as the upstream browser, and if the upstream has a range, it adds the range delimiter.
The engine check for the last release is to help remove range delimiters where there not needed. For example, if the resulting version is Edge 79, we know that it's pointless to make it a range ("≤79") since Edge 18 and earlier used a different browser engine. As such, this change now produces the following:
Caveat: due to this change, some data can no longer be mirrored for Firefox Android and Opera Android. This is a minor caveat, however, since they'll be automatically fixed once the ranges are replaced with proper version numbers.
Fixes #23855 (closes #23856).