search

rock-core / autoproj-daemon

An Autoproj daemon-plugin that watches github repositories
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

feat: fix merge branch support on GitHub #64

Closed doudou closed 2 years ago

doudou commented 2 years ago

After the refactoring for GitLab support, GitHub's support to use merge branches was broken. The issue was that the GH API does not return the "mergeable" flag in its PR list endpoint, only on the per-pull request GET endpoint.

I started fixing this by looking for the merge_commit_sha field that does exist in the listing endpoint. Turns out that this is not enough, as GH does not re-compute the merge commit in some cases. We have to actually hit the pull request GET endpoint to force the update.

At that point, ended up having issues with faraday-cache. The GET endpoint was being cached, which meant that we were not triggering the merge commit computation and were waiting forever for it to be updated. I disabled caching for this endpoint - using a separate octokit client for it. Originally, I disabled for the whole daemon but that would be triggering rate limits regularly.

g-arjones commented 2 years ago

Could you please explain what you are trying to improve in the end-user experience and/or your use case? I think it's reasonable to choose to always build the head commit but, in the context of the daemon, always building the merge commit sounds more like a bug to me since that might not be available or may not be what the user expects. I also don't understand why you override GitHub's mergeable status... I think you may build an outdated commit if a new push has conflicts with the base branch (in which case mergeable will be false and merge_commit_sha will point to the previous merge commit sha, if any).

doudou commented 2 years ago

You can find explanation about all of this in the two commit messages. If you prefer, I can copy/paste both in the pull request message. But since there are only two, I thought I could leave it this way.

g-arjones commented 2 years ago

You can find explanation about all of this in the two commit messages

I missed that, sorry. I get it now.

doudou commented 2 years ago

@g-arjones turned out that there were other issues related to merge commits. I pushed significant changes to fix that, and updated the PR message to reflect what happened. Could you please have a look ?

doudou commented 2 years ago

@g-arjones another round. Turns out this whole thing is a rabbit hole.

GitHub's rate limit is ~80 queries per minute. Our daemon does trigger it - not often, but regularly. The new commits add a mergeability cache, re-enable the HTTP cache for the all endpoints except the pull request one, and refactor dependency handling to not do any client query.

The latter bit is not needed now that I reenabled caching, but since I implemented it I'd like to keep it. I think it makes things a lot clearer (and is a small step towards new functionality I was hoping to implement in the future regarding how dependent builds are handled / represented)

g-arjones commented 2 years ago

I'm going to have a deeper look into this later but I'm not sure about the new handling of pull request dependencies because:

  1. This assumes that all pull requests from all packages are fetched before the dependencies are resolved, which may not be obvious for the reader and may not be the case for features that I also plan for the future (i.e support webhooks);
  2. I must be able to generate overrides for packages that are in the workspace but are not being watched by the daemon (a.k.a are not in the list of "known" PRs that you pass to the OverridesRetriever)

Plus, with cache enabled I don't think the change has any meaningful impact on performance

doudou commented 2 years ago

Funny, because I'm going for a completely opposite direction.

I want a central repository of all the packages, repositories, pull requests and their dependencies as a graph. The three major features that it would enable are:

To me, this is also a MUCH better design. It's a lot simpler.

And I don't think it's a bad desig with what you want, actually.

  1. for the "URLs outside the current workspace": actually, you will have to resolve dependencies early (and not on overrides generation) so that you can inject the "new repos" into the whole process. This PR definitely helps. I'd even say that any design that does not centralize the dependency information will be a nightmarish plate of spaghetti.
  2. I don't see the problem there either. This PR's design handles very well that case. Update the dependencies of the pull request object "modified" by the hook and run the rest of the process.
doudou commented 2 years ago

@g-arjones if you still do not want the dependency refactoring, I'll just move it to another PR

g-arjones commented 2 years ago

if you still do not want the dependency refactoring, I'll just move it to another PR

No, I had another look and it's fine. It's just that my goal was to move the dependencies resolution to another plugin and generate overrides during build, which would, among other things, allow testing PRs on the buildconf and testing new packages. Having to fetch pull requests from all packages in that context doesn't make sense in my opinion.

But, in the context of the daemon, I think the new design looks good. Sorry for all the noise.

g-arjones commented 2 years ago 
                    it "handles cycles in the pull requests dependencies" do
                        pr0 = PullRequest.new("0", {})
                        pr1 = PullRequest.new("1", {})
                        pr2 = PullRequest.new("2", {})

                        pr0.dependencies = [pr1, pr2]
                        pr1.dependencies = [pr2]
                        pr2.dependencies = [pr0]

                        assert_equal Set[pr1, pr2], pr0.recursive_dependencies.to_set
                        assert_equal Set[pr0, pr2], pr1.recursive_dependencies.to_set
                        assert_equal Set[pr0, pr1], pr2.recursive_dependencies.to_set
                    end

This should pass, right?

doudou commented 2 years ago

This should pass, right?

It does, doesn't it ?

g-arjones commented 2 years ago

It does, doesn't it ?

No.

doudou commented 2 years ago

??? The tests do pass.... Both locally and here. I don't get it.

doudou commented 2 years ago

??? The tests do pass.... Both locally and here. I don't get it.

I mean "I don't get what you're saying". The failure I did have looked like a very weird GH-action specific failure in atomic_write.

g-arjones commented 2 years ago

??? The tests do pass.... Both locally and here. I don't get it.

I mean the exact test case that I sent where the PullRequest URLs are different.

                    it "handles cycles in the pull requests dependencies" do
                        pr0 = PullRequest.new("0", {})
                        pr1 = PullRequest.new("1", {})
                        pr2 = PullRequest.new("2", {})

                        pr0.dependencies = [pr1, pr2]
                        pr1.dependencies = [pr2]
                        pr2.dependencies = [pr0]

                        assert_equal Set[pr1, pr2], pr0.recursive_dependencies.to_set
                        assert_equal Set[pr0, pr2], pr1.recursive_dependencies.to_set
                        assert_equal Set[pr0, pr1], pr2.recursive_dependencies.to_set
                    end
doudou commented 2 years ago

Ah .... right ... Did not notice it was different from the actual test case.

Anyways, fixed. Thanks for catching this !

g-arjones commented 2 years ago

Now I see recursion. I was really puzzled how this thing would work if you were never calling #dependencies on the dependencies... :+1:

Looks good to me.