Open wzzrd opened 3 years ago
wzzrd
commented
3 years ago I realize you have probably tested this on your own set up, so I figure we'll probably have to do some more digging and find a way to make this work for PRs within the same repo and PRs between repos.
pampersrocker
commented
3 years ago The head sha could be correct, the other changes would be identical to the old behaviour, see literally 2 lines down from your changes.
wzzrd
commented
3 years ago Mmh. Looking at b4d11a705836 I see what you mean. Bit painful.
I was really excited about the 1.6.0 release, and when it didn't work for me, tried to make it work. As said, I didn't dive into the code really deep. Probably should have.
However, with 1.6.0 I did get status updates in the wrong place. Looking at my PR in BB, head_repository points at the source repo, and I assume that head_reponame goes with that. But I admittedly still struggle with BuildBots terminology...
pampersrocker
commented
3 years ago Those are basically terminology from gitea/git. Gitea sends the json for a pull request from a fork, see below.
This wants to pull from test_org/webhook_test_fork and merge into testuser/webhook_test. From my understanding head/repo is the one to set the status at. (Aka the repo who has the latest changes that want to be merged), The base repo is the target of the pull request, in which the changes will be merged into, if the pr is accepted.
The repository is the repo on which the pr itself is located.
It might be using the wrong SHA (have not tested the SHA thoroughly), but the repo should be correct, given the assumptions above.
{
"secret": "test",
"action": "opened",
"number": 4,
"pull_request": {
"id": 36,
"url": "https://git.example.com/testuser/webhook_test/pulls/4",
"number": 4,
"user": {
"id": 1,
"login": "testuser",
"full_name": "testuser name",
"email": "testuser@example.com",
"avatar_url": "https://git.example.com/user/avatar/testuser/-1",
"language": "en-US",
"is_admin": true,
"last_login": "2021-03-27T13:53:28Z",
"created": "2018-06-05T09:41:06Z",
"username": "testuser"
},
"title": "testfork",
"body": "Test PR",
"labels": [],
"milestone": null,
"assignee": null,
"assignees": null,
"state": "open",
"is_locked": false,
"comments": 0,
"html_url": "https://git.example.com/testuser/webhook_test/pulls/4",
"diff_url": "https://git.example.com/testuser/webhook_test/pulls/4.diff",
"patch_url": "https://git.example.com/testuser/webhook_test/pulls/4.patch",
"mergeable": true,
"merged": false,
"merged_at": null,
"merge_commit_sha": null,
"merged_by": null,
"base": {
"label": "master",
"ref": "master",
"sha": "449a5a8ca05607106b5ba41988c1a658a8949a18",
"repo_id": 20,
"repo": {
"id": 20,
"owner": {
"id": 1,
"login": "testuser",
"full_name": "testuser name",
"email": "testuser@example.com",
"avatar_url": "https://git.example.com/user/avatar/testuser/-1",
"language": "en-US",
"is_admin": true,
"last_login": "2021-03-27T13:53:28Z",
"created": "2018-06-05T09:41:06Z",
"username": "testuser"
},
"name": "webhook_test",
"full_name": "testuser/webhook_test",
"description": "",
"empty": false,
"private": true,
"fork": false,
"template": false,
"parent": null,
"mirror": false,
"size": 76,
"html_url": "https://git.example.com/testuser/webhook_test",
"ssh_url": "ssh://git@git.example.com/testuser/webhook_test.git",
"clone_url": "https://git.example.com/testuser/webhook_test.git",
"original_url": "",
"website": "",
"stars_count": 0,
"forks_count": 1,
"watchers_count": 1,
"open_issues_count": 0,
"open_pr_counter": 1,
"release_counter": 0,
"default_branch": "master",
"archived": false,
"created_at": "2018-09-04T10:45:23Z",
"updated_at": "2018-09-04T13:05:51Z",
"permissions": {
"admin": false,
"push": false,
"pull": false
},
"has_issues": true,
"internal_tracker": {
"enable_time_tracker": false,
"allow_only_contributors_to_track_time": true,
"enable_issue_dependencies": true
},
"has_wiki": true,
"has_pull_requests": true,
"has_projects": false,
"ignore_whitespace_conflicts": false,
"allow_merge_commits": true,
"allow_rebase": true,
"allow_rebase_explicit": true,
"allow_squash_merge": true,
"avatar_url": "",
"internal": false
}
},
"head": {
"label": "feature_branch",
"ref": "feature_branch",
"sha": "53e3075cbe468f14c2801d186d703e64b2adee12",
"repo_id": 34,
"repo": {
"id": 34,
"owner": {
"id": 14,
"login": "test_org",
"full_name": "",
"email": "",
"avatar_url": "https://git.example.com/user/avatar/test_org/-1",
"language": "",
"is_admin": false,
"last_login": "1970-01-01T00:00:00Z",
"created": "2018-09-27T11:35:41Z",
"username": "test_org"
},
"name": "webhook_test_fork",
"full_name": "test_org/webhook_test_fork",
"description": "",
"empty": false,
"private": true,
"fork": true,
"template": false,
"parent": {
"id": 20,
"owner": {
"id": 1,
"login": "testuser",
"full_name": "testuser name",
"email": "testuser@example.com",
"avatar_url": "https://git.example.com/user/avatar/testuser/-1",
"language": "en-US",
"is_admin": true,
"last_login": "2021-03-27T13:53:28Z",
"created": "2018-06-05T09:41:06Z",
"username": "testuser"
},
"name": "webhook_test",
"full_name": "testuser/webhook_test",
"description": "",
"empty": false,
"private": true,
"fork": false,
"template": false,
"parent": null,
"mirror": false,
"size": 76,
"html_url": "https://git.example.com/testuser/webhook_test",
"ssh_url": "ssh://git@git.example.com/testuser/webhook_test.git",
"clone_url": "https://git.example.com/testuser/webhook_test.git",
"original_url": "",
"website": "",
"stars_count": 0,
"forks_count": 1,
"watchers_count": 1,
"open_issues_count": 0,
"open_pr_counter": 2,
"release_counter": 0,
"default_branch": "master",
"archived": false,
"created_at": "2018-09-04T10:45:23Z",
"updated_at": "2018-09-04T13:05:51Z",
"permissions": {
"admin": false,
"push": false,
"pull": false
},
"has_issues": true,
"internal_tracker": {
"enable_time_tracker": false,
"allow_only_contributors_to_track_time": true,
"enable_issue_dependencies": true
},
"has_wiki": true,
"has_pull_requests": true,
"has_projects": false,
"ignore_whitespace_conflicts": false,
"allow_merge_commits": true,
"allow_rebase": true,
"allow_rebase_explicit": true,
"allow_squash_merge": true,
"avatar_url": "",
"internal": false
},
"mirror": false,
"size": 19,
"html_url": "https://git.example.com/test_org/webhook_test_fork",
"ssh_url": "ssh://git@git.example.com/test_org/webhook_test_fork.git",
"clone_url": "https://git.example.com/test_org/webhook_test_fork.git",
"original_url": "",
"website": "",
"stars_count": 0,
"forks_count": 0,
"watchers_count": 1,
"open_issues_count": 0,
"open_pr_counter": 0,
"release_counter": 0,
"default_branch": "master",
"archived": false,
"created_at": "2021-03-28T21:40:46Z",
"updated_at": "2021-03-28T21:41:01Z",
"permissions": {
"admin": false,
"push": false,
"pull": false
},
"has_issues": true,
"internal_tracker": {
"enable_time_tracker": false,
"allow_only_contributors_to_track_time": true,
"enable_issue_dependencies": true
},
"has_wiki": true,
"has_pull_requests": true,
"has_projects": true,
"ignore_whitespace_conflicts": false,
"allow_merge_commits": true,
"allow_rebase": true,
"allow_rebase_explicit": true,
"allow_squash_merge": true,
"avatar_url": "",
"internal": false
}
},
"merge_base": "449a5a8ca05607106b5ba41988c1a658a8949a18",
"due_date": null,
"created_at": "2021-03-28T21:41:24Z",
"updated_at": "2021-03-28T21:41:24Z",
"closed_at": null
},
"repository": {
"id": 20,
"owner": {
"id": 1,
"login": "testuser",
"full_name": "testuser name",
"email": "testuser@example.com",
"avatar_url": "https://git.example.com/user/avatar/testuser/-1",
"language": "en-US",
"is_admin": true,
"last_login": "2021-03-27T13:53:28Z",
"created": "2018-06-05T09:41:06Z",
"username": "testuser"
},
"name": "webhook_test",
"full_name": "testuser/webhook_test",
"description": "",
"empty": false,
"private": true,
"fork": false,
"template": false,
"parent": null,
"mirror": false,
"size": 76,
"html_url": "https://git.example.com/testuser/webhook_test",
"ssh_url": "ssh://git@git.example.com/testuser/webhook_test.git",
"clone_url": "https://git.example.com/testuser/webhook_test.git",
"original_url": "",
"website": "",
"stars_count": 0,
"forks_count": 1,
"watchers_count": 1,
"open_issues_count": 0,
"open_pr_counter": 2,
"release_counter": 0,
"default_branch": "master",
"archived": false,
"created_at": "2018-09-04T10:45:23Z",
"updated_at": "2018-09-04T13:05:51Z",
"permissions": {
"admin": true,
"push": true,
"pull": true
},
"has_issues": true,
"internal_tracker": {
"enable_time_tracker": false,
"allow_only_contributors_to_track_time": true,
"enable_issue_dependencies": true
},
"has_wiki": true,
"has_pull_requests": true,
"has_projects": false,
"ignore_whitespace_conflicts": false,
"allow_merge_commits": true,
"allow_rebase": true,
"allow_rebase_explicit": true,
"allow_squash_merge": true,
"avatar_url": "",
"internal": false
},
"sender": {
"id": 1,
"login": "testuser",
"full_name": "testuser name",
"email": "testuser@example.com",
"avatar_url": "https://git.example.com/user/avatar/testuser/-1",
"language": "en-US",
"is_admin": true,
"last_login": "2021-03-27T13:53:28Z",
"created": "2018-06-05T09:41:06Z",
"username": "testuser"
},
"review": null
}Ok, I think I made the head_sha change last, so I'll check and see if with reverting the second change it still works tomorrow. Will report back after.
Thanks @pampersrocker
wzzrd
commented
3 years ago Ok I tested, because why test tomorrow if you can test today.
Sadly, it needs both changes to work for me.
I'm with you for most part of what you write above, but if the status is reported against head/repo, then the test result from buildbot will show up on the source repo (34). That might make sense in some cases, I guess, but I would expect the results to be reported against the target repo (20), don't you think? Isn't that what GitHub does? Or am I misreading the json?
(Also I was confused about the concept of a sourcestamp in buildbot, but let's let that slide for now ;) )
pampersrocker
commented
3 years ago From my limited testing, I need to set the status of the head SHA in the PR in the head repo (34 in that case) for it to show up as build status in the PR. (Case 1)
Setting the base SHA of the base repo (20) would set a working master branch as not building, because someone decided to make a PR based on that commit. (Case 2)
Setting the head SHA on the base repo (20) does not really make sense, because that SHA does not exist yet on the base repo (as it needs to be merged first). (Case 3)
Setting the base SHA on the head repo (34) would set an older commit of the fork as not building. (Case 4)
The 1.6.0 changes basically from Case 2 to Case 4 in a PR scenario, but ideally we need Case 1, which is your SHA change of this pull request.
pampersrocker
commented
3 years ago Pleas note. i've committed the head SHA change to the master and made Version v1.7.0 out of it, but had no time to investigate the fork situation further.
Having played with the 1.6.0 version a bit, I found that I had to change the repository_owner and repository_name name from head_reponame and head_owner to repository_name and owner, respectively.
Originally, I got the status reported on the source repository, on the previous commit in my repo. What I wanted was to have the status reported on the PR on the target repo.
I admit I haven't done a huge amount of reading of the code, but what it looks like to me is that head_reponame and head_owner are refering to the source repo, so I changed those to repository_name and owner, which - at least for my setup - point to the target repository of the PR.
Furthermore, in the 1.6.0 code, sha was set to
sourcestamp['revision'], which for me points to the previous commit in the repo, meaning the status would be reported on the wrong commit in case of a PR.So for PRs (which I think all have 'head_sha' in props), I am setting sha to
props['head_sha'].The result of this change for me is that the status of PR testing is reported on the target repo on the correct commit (so showing up as green checkmarks / yellow balls / red crosses on the PR).
For normal commits on this same repo, this also works.