amilner42/viva-doc

Pull Request made against pushed-forward base branch

Closed this issue · 1 comments

amilner42 commented

Currently the app relies on the pull request being opened while the HEAD of the base branch is still lined up with the pull request split.

If the HEAD of the base branch has moved forward before the pull request is opened, the github API will report that to be the PR_BASE_COMMIT and then the app will error as it is unable to find a path from the HEAD commit of feature-branch to the PR-base-commit. This was unexpected functionallity from the github api (how many times...).

Perhaps, regardless of this case, it is worth detecting when the base branch is ahead of where the pull request split and giving it a failing status so that people cannot click the "merge" button on github which will auto-resolve conflicts if possible and avoid the needed documentation checks.

  • An alternative to this is playing around with the merge_commit_sha and evaluating against that commit, the issue with that is that will be confusing as that is a commit that people don't even have locally.
  • TODO think about this
amilner42 commented

A couple thoughts:

  • I don't think it makes sense to work with the merge commit. I do want people to be encouraged to keep the docs up to date all the time, including throughout the PR, and if it requires that it be mergable then it would not be able to analyze the docs at all if the base branch gets pushed forward in a conflicting way.

This of course leaves the issue open of what if it can be auto-merged on github and so people click that avoiding actually checking the documentation on the merge?

  • I think in many cases this actually wouldn't be a problem because if it can auto-merge it seems unlikely that it would break the docs unless the person broke the docs on the master branch but that is not the point of analyzing the PR. That being said, it is possible that people would not want this, and so, it seems not hard at all for VivaDoc to allow "all docs have been approved but you must merge with the base branch before merging the commit" to prevent the auto-merges on github, thereby forcing people to evaluate the docs of the merge. This seems like a reasonable setting that some people may want.