-
Notifications
You must be signed in to change notification settings - Fork 365
Bug 1949264 - Add git migration compatibility for ./mach try perf #8611
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
base: master
Are you sure you want to change the base?
Bug 1949264 - Add git migration compatibility for ./mach try perf #8611
Conversation
As we are migrating to github away from mercurial we will not be able to see the try revision when we run ./mach try perf. This patch should allow us to be re-directed to the correct compareview links that ./mach try perf will output
As we are migrating to github away from mercurial we will not be able to see the try revision when we run ./mach try perf. This patch should allow us to be re-directed to the correct compareview links that ./mach try perf will output
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
Hi @beatrice-acasandrei, Background: Currently we use mercurial locally as a version contral system for developers, as mozilla-central will move away from mercurial and move to github repos we must adjust certain parts of our codebase that will rely exclusively on mercurial, this is one of those things. The problem: When using The solution: Treeherder has APIs with an ability to search commit messages for keywords. So when we push the changes to try we will attach a hash consisting of time, author and local commit to generate a unique identifier we can search to find commits and to generate immediate perma-links developers can access when they push to ./mach try perf. The perma link will consist of newHash and baseHash instead of newRev and baseRev. The api we create here will only be used for perfcompare(and potentially for compareview if we decide to keep it after our work wek discussions) to lookup what the revision is for each of the hashes we provide it. |
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
newpush = Commit.objects.filter(comments__contains=newhash).first() | ||
basepush = Commit.objects.filter(comments__contains=basehash).first() | ||
if newpush is None or basepush is None: | ||
raise exceptions.ValidationError( |
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.
I wonder if this should be provided to the user in a response? @beatrice-acasandrei what do you think?
test that we have a sane error when the repository does not exist | ||
""" | ||
basehash = "Invalid" | ||
newhash = "124898925481" |
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.
Is the hash always numeric? I thought it was alphanumeric.
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.
In the mozilla unified phabricator patch I use the python hash function and as per https://docs.python.org/3/library/functions.html#hash the hash returned is an integer which is numeric not alphanumeric
treeherder/webapp/api/hash.py
Outdated
raise exceptions.ValidationError( | ||
f"{newhash} or {basehash} do not correspond to any existing hashes please double check both hashes you provided" | ||
) | ||
return Response({"originalRevision": basepush.revision, "newRevision": newpush.revision}) |
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.
Is there a reason you're using originalRevision here instead of baseRevision? I think using baseRevision would be better to keep naming consistent.
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.
I don't know why I chose originalRevision over baseRevision but I'll remove it
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.
Changed to be baseRevision now
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
…m/Andrej1198/treeherder into Bug1949264_UUID_for_mach_try_perf
As we are migrating to github away from mercurial we will not be able to see the try revision when we run ./mach try perf. This patch should allow us to be re-directed to the correct compareview links that ./mach try perf will output
Should be used in conjunction with this patch to test locally: https://phabricator.services.mozilla.com/D244392