Skip to content

Sync fork prior to creating reference and handle NotFoundException#289

Merged
ryfu-msft merged 3 commits intomicrosoft:mainfrom
ryfu-msft:notFoundException
Aug 3, 2022
Merged

Sync fork prior to creating reference and handle NotFoundException#289
ryfu-msft merged 3 commits intomicrosoft:mainfrom
ryfu-msft:notFoundException

Conversation

@ryfu-msft
Copy link
Copy Markdown
Contributor

@ryfu-msft ryfu-msft commented Jul 22, 2022

Fixes #287

A NotFoundException occurs when the forked repo is behind by too many commits. This fails even when attempting to sync the forked repo with the upstream which nullifies my previous fix #235

To address this issue, I have added the following changes:

  1. Always attempt to sync fork but make it nonblocking if it fails as creating a reference could still possibly succeed. (Example: unable to sync fork because it is a fast-forward update)
  2. Add a friendly error message if a NotFoundException is encountered instructing the user to manually sync their fork.

Current tests should verify that submission succeeds as this is a minor edge case that is difficult to replicate due to the requirement of the fork needing to be behind by X amount of commits.

Microsoft Reviewers: Open in CodeFlow

@ryfu-msft ryfu-msft requested review from a team and yao-msft and removed request for a team July 22, 2022 19:07
@ghost ghost added the Issue-Bug label Jul 22, 2022
@Trenly
Copy link
Copy Markdown
Contributor

Trenly commented Jul 22, 2022

I'm not sure how winget-create handles the forks and branches, but given that we have the upstream information in order to update the fork, couldn't the commit be applied directly to a detached head which is the latest commit to the default branch of upstream, removing the need to check if the fork is behind at all?

@ryfu-msft
Copy link
Copy Markdown
Contributor Author

I'm not sure how winget-create handles the forks and branches, but given that we have the upstream information in order to update the fork, couldn't the commit be applied directly to a detached head which is the latest commit to the default branch of upstream, removing the need to check if the fork is behind at all?

Linking to the source code in case you were curious how it was implemented:

private async Task<PullRequest> SubmitPRAsync(string packageId, string version, Dictionary<string, string> contents, bool submitToFork)
{
bool createdRepo = false;
Repository repo;
if (submitToFork)
{
try
{
var user = await this.github.User.Current();
repo = await this.github.Repository.Get(user.Login, this.wingetRepo);
}
catch (NotFoundException)
{
repo = await this.github.Repository.Forks.Create(this.wingetRepoOwner, this.wingetRepo, new NewRepositoryFork());
createdRepo = true;
}
}
else
{
repo = await this.github.Repository.Get(this.wingetRepoOwner, this.wingetRepo);
}
string newBranchName = $"autogenerated/{packageId}/{Guid.NewGuid()}";
string newBranchNameHeads = $"heads/{newBranchName}";
string message = $"{packageId} version {version}";
var upstreamMaster = await this.github.Git.Reference.Get(this.wingetRepoOwner, this.wingetRepo, HeadMasterRef);
var upstreamMasterSha = upstreamMaster.Object.Sha;
Reference newBranch = null;
try
{
var retryPolicy = Policy.Handle<ApiException>().WaitAndRetryAsync(3, i => TimeSpan.FromSeconds(i));
await retryPolicy.ExecuteAsync(async () =>
{
try
{
await this.github.Git.Reference.Create(repo.Id, new NewReference($"refs/{newBranchNameHeads}", upstreamMasterSha));
}
catch (Octokit.NotFoundException)
{
// Creating a reference can sometimes fail with a NotFoundException if the fork is not up to date with the upstream repository.
await this.UpdateForkedRepoWithUpstreamCommits(repo);
throw;
}
});
// Update from upstream branch master
newBranch = await this.github.Git.Reference.Update(repo.Id, newBranchNameHeads, new ReferenceUpdate(upstreamMasterSha));
var updatedSha = newBranch.Object.Sha;
var nt = new NewTree { BaseTree = updatedSha };
string appPath = Utils.GetAppManifestDirPath(packageId, version, '/');
foreach (KeyValuePair<string, string> item in contents)
{
string file = $"{appPath}/{item.Key}.yaml";
nt.Tree.Add(new NewTreeItem { Path = file, Mode = "100644", Type = TreeType.Blob, Content = item.Value });
}
var newTree = await this.github.Git.Tree.Create(repo.Id, nt);
var newCommit = new NewCommit(message, newTree.Sha, updatedSha);
var commit = await this.github.Git.Commit.Create(repo.Id, newCommit);
await this.github.Git.Reference.Update(repo.Id, newBranchNameHeads, new ReferenceUpdate(commit.Sha));
// Get latest description template from repo
string description = await this.GetFileContentsAsync(PRDescriptionRepoPath);
string targetBranch = submitToFork ? repo.Parent.DefaultBranch : repo.DefaultBranch;
var newPullRequest = new NewPullRequest(message, $"{repo.Owner.Login}:{newBranchName}", targetBranch) { Body = description };
var pullRequest = await this.github.PullRequest.Create(this.wingetRepoOwner, this.wingetRepo, newPullRequest);
return pullRequest;
}
catch (Exception)
{
// On error, cleanup created branch/repo before re-throwing
if (createdRepo)
{
try
{
await this.github.Repository.Delete(repo.Id);
}
catch (ForbiddenException)
{
// If we fail to delete the fork, the user did not provide a token with the "delete_repo" permission. Do nothing.
}
}
else if (newBranch != null)
{
await this.github.Git.Reference.Delete(repo.Id, newBranch.Ref);
}
throw;
}
}

We already assign the new reference with the latest SHA1 value directly from the upstream default branch before attempting to create it. Even though we don't interact with the forked repo except when creating the new branch/reference, it seems like an outdated fork unintentionally impacts the Reference.Create() function and causes it to fail.

@ryfu-msft ryfu-msft merged commit 9963de9 into microsoft:main Aug 3, 2022
@ryfu-msft ryfu-msft deleted the notFoundException branch August 3, 2022 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NotFoundException error during pull request creation

3 participants