Bug 224514 – [GitHub] Add script to verify integrity of the mirror

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

It's relatively easy to verify if the GitHub mirror is out of sync with Subversion using identifiers. There is a desire to codify this check and run it automatically in infrastructure.

[Radar WebKit Bug Importer]

<rdar://problem/76613576>

[Jonathan Bedard]

Created attachment 425923 [details]
Patch

[Aakash Jain]

Comment on attachment 425923 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425923&action=review

> Tools/Scripts/check-github-mirror-integrity:39
> +        print(f'Cannonical HEAD commit {canonical_tip} matches mirrored HEAD commit {mirror_tip}')

It might be confusing to users (like me) to clearly understand what exactly "Cannonical" and "mirrored" refer to. Might be better to directly refer to GitHub and svn.webkit.org in all the output messages. Maybe something like this would be better:

print(f'GitHub and svn.webkit.org are in sync.\nHEAD commit on GitHub {canonical_tip} matches HEAD commit on svn.webkit.org {mirror_tip}')

> Tools/Scripts/check-github-mirror-integrity:42
> +    if mirror_tip.identifier == canonical_tip.identifier - 1:

Why is this fine, and in which case do we expect this?

> Tools/Scripts/check-github-mirror-integrity:43
> +        print(f'Cannonical HEAD commit {canonical_tip} slitly ahead of mirrored HEAD commit {mirror_tip}')

Should reverse the message and make it more user-friendly. Something like:
print(f'GitHub repo is slightly behind svn.webkit.org. This is ok.\nHEAD commit on GitHub {canonical_tip} is one behind HEAD commit on svn.webkit.org {mirror_tip}')

> Tools/Scripts/check-github-mirror-integrity:46
> +    print(f'Cannonical HEAD commit {canonical_tip} does not match mirrored HEAD commit {mirror_tip}')

Better to start this message with: "Error: ", so that it's clear to the user that this is an error condition. 
Might as well list the next steps. e.g.: "This indicates a serious issue with the repository, please inform [e-mail] urgently."

[Jonathan Bedard]

Comment on attachment 425923 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425923&action=review

>> Tools/Scripts/check-github-mirror-integrity:42
>> +    if mirror_tip.identifier == canonical_tip.identifier - 1:
> 
> Why is this fine, and in which case do we expect this?

I'm concerned if we don't do this, we'll find ourselves with a common race condition where we are in the process of running the post-commit hook but haven't yet pushed to GitHub.

[Aakash Jain]

Comment on attachment 425923 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425923&action=review

>> Tools/Scripts/check-github-mirror-integrity:46
>> +    print(f'Cannonical HEAD commit {canonical_tip} does not match mirrored HEAD commit {mirror_tip}')
> 
> Better to start this message with: "Error: ", so that it's clear to the user that this is an error condition. 
> Might as well list the next steps. e.g.: "This indicates a serious issue with the repository, please inform [e-mail] urgently."

Also it would be a good idea to print how much behind is github, both, in terms of number of commits and time.

[Aakash Jain]

(In reply to Jonathan Bedard from comment #4)
> I'm concerned if we don't do this, we'll find ourselves with a common race condition where we are in the process of running the post-commit hook but haven't yet pushed to GitHub.
ok, make sense. But it might be a real issue as well, where the syncing is broken and only one commit was made after that.

It would be be a good idea to print the time delta since the first missing commit from github was made, and if that time is higher than the time we expect syncing to take, then we should print an error.

[Jonathan Bedard]

Created attachment 426011 [details]
Patch

[Aakash Jain]

Comment on attachment 426011 [details]
Patch

Doesn't address my comment about adding time based check, and adding further statement to error message like: "This indicates a serious issue with the repository, please inform [e-mail] urgently."

But those can be done later as well.

[Jonathan Bedard]

Created attachment 426021 [details]
[fast-cq] patch

[EWS]

Committed r275970 (236523@main): <https://commits.webkit.org/236523@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426021 [details].