Fork feature pr branch

[meygerjos]

In fulfillment of this issue

[eduardoboucas]

This is great, thanks! Give me some time to play with this and I'll feedback.

[eduardoboucas]

First of all, sorry for the late reply. I've just merged a PR that took all my focus for a few weeks.

Can you provide some detail on how this proposed functionality works? I see that you're adding a /fork endpoint — when and who will be calling it?

Also, what will happen with a fork after the PR is merged? Do we keep it indefinitely under the bot's account, or do we delete it? If we keep it, is there a risk that the base repository changes and our fork becomes outdated, possibly leading to merge conflicts? We'd be stuck at that point and unable to proceed without human interaction.

Thanks!

[meygerjos]

Hi!

The usage of the /fork endpoint is described in my changes to the README.md file. Similar to the /connect endpoint, it accepts a GET request that the repository owner fires manually with their browser. Upon receiving this GET request, the Staticman API tells the bot to fork the owner's repository.

If fork=true and moderation=true, then comment submission will trigger the creation of branches on the bot's fork of the owner's repository, rather than on the owner's repository directly.

It may be possible to eliminate the /fork endpoint, but I could not figure out how to do it. Here is the issue. Suppose I set fork=true and moderation=true, but I don't send the GET request to the /fork endpoint. Then the first comment that is submitted will cause the bot to make a fork of the owner's repository, but for some reason it won't make any branch on this fork. However, subsequent comments will work as expected. To get around this issue I made the /fork endpoint, since I thought it would be strange to tell users to send one garbage comment to prime the bot with a fork of their repository, but if you think this is better, I could remove the /fork endpoint and change the README.md accordingly.

Here is what is going on here that causes this error. The first time that a comment is submitted, forkAndWriteFileAndSendPR in Github.js is called. This method first calls this.api.repos.fork, which, according to the GitHub API documentation, simply gets a fork, but in practice, if such a fork does not exist, creates one. So a fork is created. Then the method creates a branch of the fork with this.api.repos.getBranch, which works, and tries to create a reference for the branch with this.api.gitdata.createReference, which fails. Why does this fail? I think it is because the Github API falsely reports back that it has fulfilled the promise of making the fork and the branch when it really hasn't yet, so that there isn't anything yet to make a reference for. However, when the second comment is submitted, there is no error, because the fork already exists and this.api.repos.fork doesn't have to take the time to make it.

In response to your second paragraph of questions: My code currently keeps a fork indefinitely, and there is no way of deleting it or syncing it. You're right that this could be a problem. Maybe the /fork endpoint should repurposed to either create a new fork or sync an existing fork. Should I make this change, or a different one?

Thanks for your feedback!

[eduardoboucas]

No, thank you for taking the time to describe your reasoning!

Then the first comment that is submitted will cause the bot to make a fork of the owner's repository, but for some reason it won't make any branch on this fork. However, subsequent comments will work as expected

I believe this is because creating a fork is an asynchronous operation, as described here. This means that even though the Promise resolves, the fork doesn't exist at that point yet.

I think this poses a series of problems, with the first one being that even if our strategy is to ask people to ping the /fork endpoint before sending any comments, there's no guarantee that the fork will have been created by the time they start sending data. This is also an issue if we try to do anything to prevent forks from being stale, i.e. we can't easily delete our fork and create a new one regularly because that's an asynchronous operation that takes an undetermined amount of time.

I can definitely see the value of adding this feature, but I worry about the potential caveats of the implementation.

[meygerjos]

I see. So why don't we inform the user that /fork is asynchronous in the README.md and tell them to wait at least 5 minutes after sending a GET request to it before expecting comments to work? Also, it isn't necessary to completely delete the fork and make a new one, we can just sync a stale fork and tell the bot to automatically resolve merge conflicts in favor of the owner's repository. I don't think this operation is asynchronous.

On another note, it would be convenient if the bot could allow edits from maintainers on pull requests it makes, but I'm not sure if the Github API has this option.

[eduardoboucas]

I must say I don't like the idea of a /fork endpoint. It's yet another step that people need to take to set up Staticman that seems unnecessary. We should be able to handle everything within the /entry endpoint.

After requesting the fork creation, I would probably poll the API every 30 seconds or so to check if the fork had been successfully created, and then proceed with the rest of the steps.

[meygerjos]

OK that sounds good. Why not every 5 seconds instead of 30, since that is the interval of time that Github specifies as an upper bound for how long it could take unless there is something wrong?

The bot could listen for changes to the owner's repository and automatically sync to them, similar to 1egoman/backstroke.