feature: check for existing updates with same src url #316
Conversation
|
Wouldn't this solution mean that a PR for Maybe a better check would be to do something like hashing the diffs created? |
ryantm
changed the title
|
@rhendric Good point, how about how it is now where it checks for a duplicate |
|
Yeah, that sounds like it would work in principle! But now there are implementation difficulties; based on my testing, GitHub's search API is too sloppy to match multiword phrases exactly, or even a single URL. Check out how returns a result in which the URL mentioned is actually That's part of why I suggested a hash; I hope that a string of only alphanumeric characters is safe to search on. We can hash the source URL, and maybe hide it in |
| where | ||
| title = U.prTitle env attrPath | ||
| search = [interpolate|repo:nixos/nixpkgs $title |] | ||
| srcUrlSearch = [interpolate|new src url: $srcUrl |] |
There was a problem hiding this comment.
This needs a repo:nixos/nixpkgs too, in whatever form it ultimately takes.
| when (srcUrl /= T.empty && T.length (openPRReport srcUrlSearchResult) /= 0) | ||
| (throwE | ||
| ( "There might already be an open PR for this update:\n" | ||
| <> openPRReport searchResult)) |
There was a problem hiding this comment.
| <> openPRReport searchResult)) | |
| <> openPRReport srcUrlSearchResult)) |
| @@ -286,7 +288,7 @@ updateAttrPath log mergeBase updateEnv@UpdateEnv {..} attrPath = do | |||
| mbLastCommitMsg <- lift $ Git.findAutoUpdateBranchMessage packageName | |||
| tell $ Alt mbLastCommitMsg | |||
| unless hasUpdateScript do | |||
| lift $ checkExistingUpdate log updateEnv mbLastCommitMsg attrPath | |||
| lift $ checkExistingUpdate log updateEnv mbLastCommitMsg attrPath oldSrcUrl | |||
There was a problem hiding this comment.
This doesn't look right to me—checking for open PRs that target the old src seems useless at best.
I figure we want to do this new check after the rewrites have run in both the hasUpdateScript and not hasUpdateScript cases, so maybe it should be pulled out of checkExistingUpdate and put in its own function.
fixes #315
@rhendric interested in reviewing this?