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

Build status should be on target repo #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wzzrd
Copy link

@wzzrd wzzrd commented May 17, 2021

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.

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.
@wzzrd
Copy link
Author

wzzrd commented May 17, 2021

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

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

wzzrd commented May 17, 2021

Mmh. Looking at b4d11a7 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
Copy link
Contributor

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": "[email protected]",
      "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": "[email protected]",
          "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://[email protected]/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": "[email protected]",
            "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://[email protected]/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://[email protected]/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": "[email protected]",
      "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://[email protected]/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": "[email protected]",
    "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
}

@wzzrd
Copy link
Author

wzzrd commented May 17, 2021

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

wzzrd commented May 17, 2021

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

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

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.

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

Successfully merging this pull request may close these issues.

2 participants