Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feature: check for existing updates with same src url #316

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feature: check for existing updates with same src url #316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 20 additions & 9 deletions src/GH.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,23 +201,34 @@ authFromToken = GH.OAuth . T.encodeUtf8
authFrom :: UpdateEnv -> GH.Auth
authFrom = authFromToken . U.githubToken . options

checkExistingUpdatePR :: MonadIO m => UpdateEnv -> Text -> ExceptT Text m ()
checkExistingUpdatePR env attrPath = do
checkExistingUpdatePR :: MonadIO m => UpdateEnv -> Text -> Text -> ExceptT Text m ()
checkExistingUpdatePR env attrPath srcUrl = do
searchResult <-
ExceptT $
liftIO $
GH.github (authFrom env) (GH.searchIssuesR search)
& fmap (first (T.pack . show))
if T.length (openPRReport searchResult) == 0
then return ()
else
throwE
( "There might already be an open PR for this update:\n"
<> openPRReport searchResult
)
when (T.length (openPRReport searchResult) /= 0)
(throwE
( "There might already be an open PR for this update:\n"
<> openPRReport searchResult))

srcUrlSearchResult <-
ExceptT $
liftIO $
GH.github (authFrom env) (GH.searchIssuesR srcUrlSearch)
& fmap (first (T.pack . show))

when (srcUrl /= T.empty && T.length (openPRReport srcUrlSearchResult) /= 0)
(throwE
( "There might already be an open PR for this update:\n"
<> openPRReport searchResult))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<> openPRReport searchResult))
<> openPRReport srcUrlSearchResult))


return ()
where
title = U.prTitle env attrPath
search = [interpolate|repo:nixos/nixpkgs $title |]
srcUrlSearch = [interpolate|new src url: $srcUrl |]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a repo:nixos/nixpkgs too, in whatever form it ultimately takes.

openPRReport searchResult =
GH.searchResultResults searchResult
& V.filter (GH.issueClosedAt >>> isNothing)
Expand Down
22 changes: 15 additions & 7 deletions src/Update.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,8 +247,9 @@ checkExistingUpdate ::
UpdateEnv ->
Maybe Text ->
Text ->
Text ->
ExceptT Text IO ()
checkExistingUpdate log updateEnv existingCommitMsg attrPath = do
checkExistingUpdate log updateEnv existingCommitMsg srcUrl attrPath = do
case existingCommitMsg of
Nothing -> lift $ log "No auto update branch exists"
Just msg -> do
Expand All @@ -263,7 +264,7 @@ checkExistingUpdate log updateEnv existingCommitMsg attrPath = do
-- Note that this check looks for PRs with the same old and new
-- version numbers, so it won't stop us from updating an existing PR
-- if this run updates the package to a newer version.
GH.checkExistingUpdatePR updateEnv attrPath
GH.checkExistingUpdatePR updateEnv attrPath srcUrl

updateAttrPath ::
(Text -> IO ()) ->
Expand All @@ -277,6 +278,7 @@ updateAttrPath log mergeBase updateEnv@UpdateEnv {..} attrPath = do

successOrFailure <- runExceptT $ do
hasUpdateScript <- Nix.hasUpdateScript attrPath
oldSrcUrl <- Nix.getSrcUrl attrPath <|> pure ""

existingCommitMsg <- fmap getAlt . execWriterT $
whenBatch updateEnv do
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


unless hasUpdateScript do
Nix.assertNewerVersion updateEnv
Expand All @@ -310,7 +312,6 @@ updateAttrPath log mergeBase updateEnv@UpdateEnv {..} attrPath = do
-- Get the original values for diffing purposes
derivationContents <- liftIO $ T.readFile $ T.unpack derivationFile
oldHash <- Nix.getOldHash attrPath <|> pure ""
oldSrcUrl <- Nix.getSrcUrl attrPath <|> pure ""
oldRev <- Nix.getAttr Nix.Raw "rev" attrPath <|> pure ""
oldVerMay <- rightMay `fmapRT` (lift $ runExceptT $ Nix.getAttr Nix.Raw "version" attrPath)

Expand Down Expand Up @@ -379,17 +380,18 @@ updateAttrPath log mergeBase updateEnv@UpdateEnv {..} attrPath = do
assertNotUpdatedOn updateEnv' derivationFile "master"
assertNotUpdatedOn updateEnv' derivationFile "staging"
assertNotUpdatedOn updateEnv' derivationFile "staging-next"

whenBatch updateEnv do
when pr do
when hasUpdateScript do
checkExistingUpdate log updateEnv' existingCommitMsg attrPath
checkExistingUpdate log updateEnv' existingCommitMsg attrPath newSrcUrl

Nix.build attrPath

--
-- Publish the result
lift . log $ "Successfully finished processing"
result <- Nix.resultLink
result <- Nix.resultLink
let opReport =
if isJust skipOutpathBase
then "Outpath calculations were skipped for this package; total number of rebuilds unknown."
Expand Down Expand Up @@ -455,6 +457,7 @@ publishPackage log updateEnv oldSrcUrl newSrcUrl attrPath result opReport prBase
metaDescription
metaHomepage
metaChangelog
newSrcUrl
rewriteMsgs
releaseUrl
compareUrl
Expand Down Expand Up @@ -494,6 +497,7 @@ prMessage ::
Text ->
Text ->
Text ->
Text ->
[Text] ->
Text ->
Text ->
Expand All @@ -507,7 +511,7 @@ prMessage ::
Text ->
Text ->
Text
prMessage updateEnv isBroken metaDescription metaHomepage metaChangelog rewriteMsgs releaseUrl compareUrl resultCheckReport commitRev attrPath maintainers resultPath opReport cveRep cachixTestInstructions nixpkgsReviewMsg =
prMessage updateEnv isBroken metaDescription metaHomepage metaChangelog newSrcUrl rewriteMsgs releaseUrl compareUrl resultCheckReport commitRev attrPath maintainers resultPath opReport cveRep cachixTestInstructions nixpkgsReviewMsg =
-- Some components of the PR description are pre-generated prior to calling
-- because they require IO, but in general try to put as much as possible for
-- the formatting into the pure function so that we can control the body
Expand Down Expand Up @@ -538,6 +542,8 @@ prMessage updateEnv isBroken metaDescription metaHomepage metaChangelog rewriteM
if compareUrl == T.empty
then ""
else "- [Compare changes on GitHub](" <> compareUrl <> ")"
-- Needed for search of existing PRs
newSrcUrlLine = "- new src url: " <> newSrcUrl
nixpkgsReviewSection =
if nixpkgsReviewMsg == T.empty
then "NixPkgs review skipped"
Expand Down Expand Up @@ -571,6 +577,8 @@ prMessage updateEnv isBroken metaDescription metaHomepage metaChangelog rewriteM

###### To inspect upstream changes

$newSrcUrlLine

$releaseUrlMessage

$compareUrlMessage
Expand Down