From 517246b40c3885112fec53edcc14655ed6863237 Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Thu, 27 May 2021 19:38:10 +0000 Subject: [PATCH 1/6] dlangbot.warnings: Delete empty function --- source/dlangbot/warnings.d | 7 ------- 1 file changed, 7 deletions(-) diff --git a/source/dlangbot/warnings.d b/source/dlangbot/warnings.d index fd313ea..72ecb63 100644 --- a/source/dlangbot/warnings.d +++ b/source/dlangbot/warnings.d @@ -14,12 +14,6 @@ struct UserMessage } -// check diff length -void checkDiff(in ref PullRequest pr, ref UserMessage[] msgs) -{ -} - - /** Check bugzilla priority - enhancement -> changelog entry @@ -55,7 +49,6 @@ git rebase --onto upstream/stable upstream/master UserMessage[] checkForWarnings(in PullRequest pr, in Issue[] bugzillaIssues, in IssueRef[] refs) { UserMessage[] msgs; - pr.checkDiff(msgs); pr.checkBugzilla(msgs, bugzillaIssues, refs); return msgs; } From bf4a75ecfa7ec45fa6178adf9ebc30a98e0cf640 Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Thu, 27 May 2021 20:02:22 +0000 Subject: [PATCH 2/6] dlang-bot.test.utils: Do not assume that all payloads are JSON Necessary in order to allow us to mock diffs. --- test/utils.d | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/test/utils.d b/test/utils.d index 4535e33..3b03779 100644 --- a/test/utils.d +++ b/test/utils.d @@ -175,6 +175,11 @@ auto payloadServer(scope HTTPServerRequest req, scope HTTPServerResponse res) } } +void replaceAPIReferences(string official, string local, ref string str) +{ + str = str.replace(official, local); +} + void replaceAPIReferences(string official, string local, ref Json json) { void recursiveReplace(ref Json j) @@ -188,7 +193,10 @@ void replaceAPIReferences(string official, string local, ref Json json) case Json.Type.string: string v = j.get!string; if (v.countUntil(official) >= 0) - j = v.replace(official, local); + { + replaceAPIReferences(official, local, v); + j = v; + } break; default: break; @@ -294,7 +302,7 @@ void postGitHubHook(string payload, string eventType = "pull_request", auto req = requestHTTP(ghTestHookURL, (scope req) { req.method = HTTPMethod.POST; - auto payload = payload.readText.parseJsonString; + auto payload = payload.readText; // localize accessed URLs replaceAPIReferences("https://api.github.com", githubAPIURL, payload); @@ -302,11 +310,14 @@ void postGitHubHook(string payload, string eventType = "pull_request", req.headers["X-GitHub-Event"] = eventType; if (postprocess !is null) - postprocess(payload, req); + { + auto payloadJson = payload.parseJsonString; + postprocess(payloadJson, req); + payload = payloadJson.toString; + } - auto respStr = payload.toString; - req.headers["X-Hub-Signature"] = getSignature(respStr); - req.writeBody(cast(ubyte[]) respStr); + req.headers["X-Hub-Signature"] = getSignature(payload); + req.writeBody(cast(ubyte[]) payload); }); assert(req.statusCode == 200, "Request failed with status %d. Response body:\n\n%s" From f2b60a9e0e4f7b150329300d74887da4fb31b7a3 Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Thu, 27 May 2021 20:03:33 +0000 Subject: [PATCH 3/6] Add support for mocking github.com Diffs are served via github.com, not api.github.com. --- source/dlangbot/app.d | 2 +- source/dlangbot/github_api.d | 1 + test/bugzilla.d | 4 ++-- test/utils.d | 5 ++++- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/source/dlangbot/app.d b/source/dlangbot/app.d index d104bd4..c8521a4 100644 --- a/source/dlangbot/app.d +++ b/source/dlangbot/app.d @@ -7,7 +7,7 @@ import dlangbot.trello; import dlangbot.utils; public import dlangbot.bugzilla : bugzillaLogin, bugzillaPassword, bugzillaURL; -public import dlangbot.github_api : githubAPIURL, githubAuth, githubHookSecret; +public import dlangbot.github_api : githubURL, githubAPIURL, githubAuth, githubHookSecret; public import dlangbot.trello : trelloAPIURL, trelloAuth, trelloSecret; public import dlangbot.twitter : oAuth, tweet, twitterURL, twitterEnabled; public import dlangbot.buildkite : buildkiteAPIURL, buildkiteAuth, buildkiteHookSecret, dlangbotAgentAuth; diff --git a/source/dlangbot/github_api.d b/source/dlangbot/github_api.d index b85a461..6adbbb9 100644 --- a/source/dlangbot/github_api.d +++ b/source/dlangbot/github_api.d @@ -1,5 +1,6 @@ module dlangbot.github_api; +shared string githubURL = "https://github.com"; shared string githubAPIURL = "https://api.github.com"; shared string githubAuth, githubHookSecret; diff --git a/test/bugzilla.d b/test/bugzilla.d index 8a94664..0c4cc33 100644 --- a/test/bugzilla.d +++ b/test/bugzilla.d @@ -225,7 +225,7 @@ unittest assert("keywords" in req.json["params"][0]); auto comment = req.json["params"][0]["comment"]["body"].get!string; - enum expected = q"EOF + auto expected = q"EOF @MartinNowak created dlang/dmd pull request #6359 "fix Issue 16794 - dmd not working on Ubuntu 16.10" fixing this issue: - fix Issue 16794 - dmd not working on Ubuntu 16.10 @@ -235,7 +235,7 @@ unittest - also see https://github.com/dlang/installer/pull/207 https://github.com/dlang/dmd/pull/6359 -EOF".chomp; +EOF".chomp.replace("https://github.com/dlang/installer", githubURL ~ "/dlang/installer"); // compensate for API reference rewriting assert(comment == expected, comment); auto j = Json(["error" : Json(null), "result" : Json.emptyObject]); diff --git a/test/utils.d b/test/utils.d index 3b03779..b72e155 100644 --- a/test/utils.d +++ b/test/utils.d @@ -83,6 +83,7 @@ void startFakeAPIServer() auto fakeAPIServerURL = "http://" ~ fakeSettings.bindAddresses[0] ~ ":" ~ fakeSettings.port.to!string; + githubURL = fakeAPIServerURL ~ "/github.com"; githubAPIURL = fakeAPIServerURL ~ "/github"; trelloAPIURL = fakeAPIServerURL ~ "/trello"; buildkiteAPIURL = fakeAPIServerURL ~ "/buildkite"; @@ -156,9 +157,10 @@ auto payloadServer(scope HTTPServerRequest req, scope HTTPServerResponse res) { logInfo("reading payload: %s", filePath); auto payload = filePath.readText; - if (req.requestURL.startsWith("/github", "/trello", "/buildkite", "/hcloud")) + if (req.requestURL.startsWith("/github/", "/trello/", "/buildkite/", "/hcloud/")) { auto payloadJson = payload.parseJsonString; + replaceAPIReferences("https://github.com", githubURL, payloadJson); replaceAPIReferences("https://api.github.com", githubAPIURL, payloadJson); replaceAPIReferences("https://api.trello.com", trelloAPIURL, payloadJson); replaceAPIReferences("https://api.buildkite.com/v2", buildkiteAPIURL, payloadJson); @@ -305,6 +307,7 @@ void postGitHubHook(string payload, string eventType = "pull_request", auto payload = payload.readText; // localize accessed URLs + replaceAPIReferences("https://github.com", githubURL, payload); replaceAPIReferences("https://api.github.com", githubAPIURL, payload); req.headers["X-GitHub-Event"] = eventType; From e03480e836fac835bb2d0fdb461c7fdaa93135ab Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Thu, 27 May 2021 20:04:18 +0000 Subject: [PATCH 4/6] dlangbot.github_api: Add PullRequest.diffURL --- source/dlangbot/github_api.d | 1 + 1 file changed, 1 insertion(+) diff --git a/source/dlangbot/github_api.d b/source/dlangbot/github_api.d index 6adbbb9..9185ad1 100644 --- a/source/dlangbot/github_api.d +++ b/source/dlangbot/github_api.d @@ -242,6 +242,7 @@ struct PullRequest @name("created_at") SysTime createdAt; @name("updated_at") SysTime updatedAt; @name("closed_at") Nullable!SysTime closedAt; + @name("diff_url") string diffURL; bool locked; // TODO: update payloads //@name("maintainer_can_modify") bool maintainerCanModify; From 3946807517acde68e0516f7df6006a5e72922c29 Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Thu, 27 May 2021 20:40:04 +0000 Subject: [PATCH 5/6] dlangbot.warnings: Warn when fixing enhancements without changelog Fixes #178. --- .../github.com_dlang_phobos_pull_4921.diff | 0 source/dlangbot/warnings.d | 22 +++++ test/comments.d | 89 +++++++++++++++++++ 3 files changed, 111 insertions(+) create mode 100644 data/payloads/github.com_dlang_phobos_pull_4921.diff diff --git a/data/payloads/github.com_dlang_phobos_pull_4921.diff b/data/payloads/github.com_dlang_phobos_pull_4921.diff new file mode 100644 index 0000000..e69de29 diff --git a/source/dlangbot/warnings.d b/source/dlangbot/warnings.d index 72ecb63..af87bbc 100644 --- a/source/dlangbot/warnings.d +++ b/source/dlangbot/warnings.d @@ -14,6 +14,27 @@ struct UserMessage } +void checkEnhancementChangelog(in ref PullRequest pr, ref UserMessage[] msgs, + in Issue[] bugzillaIssues, in IssueRef[] refs) +{ + import dlangbot.utils : request, expectOK; + import vibe.stream.operations : readAllUTF8; + + if (bugzillaIssues.any!(i => i.status.among("NEW", "ASSIGNED", "REOPENED") && + i.severity == "enhancement" && + refs.canFind!(r => r.id == i.id && r.fixed))) + { + auto diff = request(pr.diffURL).expectOK.bodyReader.readAllUTF8; + if (!diff.canFind("\n+++ b/changelog/")) + { + msgs ~= UserMessage(UserMessage.Type.Warning, + "Pull requests implementing enhancements should include a [full changelog entry](https://github.com/dlang/dmd/tree/master/changelog)." + ); + } + } +} + + /** Check bugzilla priority - enhancement -> changelog entry @@ -50,6 +71,7 @@ UserMessage[] checkForWarnings(in PullRequest pr, in Issue[] bugzillaIssues, in { UserMessage[] msgs; pr.checkBugzilla(msgs, bugzillaIssues, refs); + pr.checkEnhancementChangelog(msgs, bugzillaIssues, refs); return msgs; } diff --git a/test/comments.d b/test/comments.d index d90818f..bebacaa 100644 --- a/test/comments.d +++ b/test/comments.d @@ -131,6 +131,7 @@ unittest 8573,"A simpler Phobos function that returns the index of the mix or max item","NEW","---","enhancement","P2",`); }, "/github/repos/dlang/phobos/issues/4921/labels", + "/github.com/dlang/phobos/pull/4921.diff", "/github/repos/dlang/phobos/issues/4921/labels", (ref Json j) { j = Json.emptyArray; }, @@ -169,6 +170,7 @@ unittest 8574,"Some Bug","NEW","---","normal","P2",`); }, "/github/repos/dlang/phobos/issues/4921/labels", + "/github.com/dlang/phobos/pull/4921.diff", "/github/repos/dlang/phobos/issues/4921/labels", (ref Json j) { j = Json.emptyArray; }, @@ -200,6 +202,7 @@ unittest "/github/repos/dlang/phobos/issues/4921/comments", "/bugzilla/buglist.cgi?bug_id=8573&ctype=csv&columnlist=short_desc,bug_status,resolution,bug_severity,priority,keywords", "/github/repos/dlang/phobos/issues/4921/labels", + "/github.com/dlang/phobos/pull/4921.diff", "/github/repos/dlang/phobos/issues/4921/labels", (ref Json j) { j[0]["name"] = "Enhancement"; }, @@ -558,3 +561,89 @@ unittest assert("a\\b".markdownEscape == "a\\\\b"); assert("a+b".markdownEscape == r"a\+b"); } + +@("enhancement-with-changelog") +unittest +{ + setAPIExpectations( + "/github/repos/dlang/phobos/pulls/4921/commits", (ref Json j) { + j[0]["commit"]["message"] = "Fix Issue 8573"; + }, + "/github/repos/dlang/phobos/issues/4921/comments", + "/bugzilla/buglist.cgi?bug_id=8573&ctype=csv&columnlist=short_desc,bug_status,resolution,bug_severity,priority,keywords", + (scope HTTPServerRequest req, scope HTTPServerResponse res){ + res.writeBody( +`bug_id,"short_desc","bug_status","resolution","bug_severity","priority","keywords" +8573,"A simpler Phobos function that returns the index of the mix or max item","NEW","---","enhancement","P2",`); + }, + "/github/repos/dlang/phobos/issues/4921/labels", + "/github.com/dlang/phobos/pull/4921.diff", (scope HTTPServerRequest req, scope HTTPServerResponse res){ + res.writeBody(q"EOF +diff --git a/changelog/bla-bla.dd b/changelog/bla-bla.dd +new file mode 100644 +index 00000000000..0147f501f08 +--- /dev/null ++++ b/changelog/bla-bla.dd +@@ -0,0 +1,1 @@ ++bla bla +EOF"); + }, + "/github/repos/dlang/phobos/issues/4921/labels", (ref Json j) { + j = Json.emptyArray; + }, + "/github/orgs/dlang/public_members?per_page=100", + "/github/repos/dlang/phobos/issues/comments/262784442", + (scope HTTPServerRequest req, scope HTTPServerResponse res){ + assert(req.method == HTTPMethod.PATCH); + assert(!req.json["body"].get!string.canFind("Pull requests implementing enhancements should include")); + res.writeVoidBody; + }, + "/github/repos/dlang/phobos/issues/4921/labels", + (scope HTTPServerRequest req, scope HTTPServerResponse res){ + assert(req.json[].equal(["Enhancement"])); + }, + "/trello/1/search?query=name:%22Issue%208573%22&"~trelloAuth, + "/bugzilla/jsonrpc.cgi", // Bug.comments + "/bugzilla/jsonrpc.cgi", // Bug.update + ); + + postGitHubHook("dlang_phobos_synchronize_4921.json"); +} + +@("enhancement-with-changelog") +unittest +{ + setAPIExpectations( + "/github/repos/dlang/phobos/pulls/4921/commits", (ref Json j) { + j[0]["commit"]["message"] = "Fix Issue 8573"; + }, + "/github/repos/dlang/phobos/issues/4921/comments", + "/bugzilla/buglist.cgi?bug_id=8573&ctype=csv&columnlist=short_desc,bug_status,resolution,bug_severity,priority,keywords", + (scope HTTPServerRequest req, scope HTTPServerResponse res){ + res.writeBody( +`bug_id,"short_desc","bug_status","resolution","bug_severity","priority","keywords" +8573,"A simpler Phobos function that returns the index of the mix or max item","NEW","---","enhancement","P2",`); + }, + "/github/repos/dlang/phobos/issues/4921/labels", + "/github.com/dlang/phobos/pull/4921.diff", + "/github/repos/dlang/phobos/issues/4921/labels", (ref Json j) { + j = Json.emptyArray; + }, + "/github/orgs/dlang/public_members?per_page=100", + "/github/repos/dlang/phobos/issues/comments/262784442", + (scope HTTPServerRequest req, scope HTTPServerResponse res){ + assert(req.method == HTTPMethod.PATCH); + assert(req.json["body"].get!string.canFind("Pull requests implementing enhancements should include")); + res.writeVoidBody; + }, + "/github/repos/dlang/phobos/issues/4921/labels", + (scope HTTPServerRequest req, scope HTTPServerResponse res){ + assert(req.json[].equal(["Enhancement"])); + }, + "/trello/1/search?query=name:%22Issue%208573%22&"~trelloAuth, + "/bugzilla/jsonrpc.cgi", // Bug.comments + "/bugzilla/jsonrpc.cgi", // Bug.update + ); + + postGitHubHook("dlang_phobos_synchronize_4921.json"); +} From d52ecbe8b035ec6ca150246d3c8d60b6e9eb108d Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Fri, 28 May 2021 03:13:01 +0000 Subject: [PATCH 6/6] Use the same logic for calculating the manual changelog URL --- source/dlangbot/github.d | 11 +++++++++-- source/dlangbot/warnings.d | 6 ++++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/source/dlangbot/github.d b/source/dlangbot/github.d index 5d25ca0..237a47c 100644 --- a/source/dlangbot/github.d +++ b/source/dlangbot/github.d @@ -52,6 +52,13 @@ string markdownEscape(string desc) return app.data; } +string manualChangelogURL(string repoSlug) +{ + return "https://github.com/%s/blob/master/%s".format( + repoSlug, repoSlug == "dlang/dlang.org" ? "language-changelog" : "changelog", + ); +} + string formatComment(in ref PullRequest pr, in IssueRef[] refs, in Issue[] descs, in UserMessage[] msgs) { import std.array : appender; @@ -101,8 +108,8 @@ If you have addressed all reviews or aren't sure how to proceed, don't hesitate app.formattedWrite( `Your PR doesn't reference any Bugzilla issue. -If your PR contains non-trivial changes, please [reference a Bugzilla issue](https://github.com/dlang-bots/dlang-bot#automated-references) or create a [manual changelog](https://github.com/%s/blob/master/%s). -`, pr.repoSlug, pr.repoSlug == "dlang/dlang.org" ? "language-changelog" : "changelog"); +If your PR contains non-trivial changes, please [reference a Bugzilla issue](https://github.com/dlang-bots/dlang-bot#automated-references) or create a [manual changelog](%s). +`, manualChangelogURL(pr.repoSlug)); } if (msgs.length) diff --git a/source/dlangbot/warnings.d b/source/dlangbot/warnings.d index af87bbc..7c1a09a 100644 --- a/source/dlangbot/warnings.d +++ b/source/dlangbot/warnings.d @@ -1,7 +1,7 @@ module dlangbot.warnings; import dlangbot.bugzilla : Issue, IssueRef; -import dlangbot.github : PullRequest; +import dlangbot.github : PullRequest, manualChangelogURL; import std.algorithm; @@ -19,6 +19,7 @@ void checkEnhancementChangelog(in ref PullRequest pr, ref UserMessage[] msgs, { import dlangbot.utils : request, expectOK; import vibe.stream.operations : readAllUTF8; + import std.format : format; if (bugzillaIssues.any!(i => i.status.among("NEW", "ASSIGNED", "REOPENED") && i.severity == "enhancement" && @@ -28,7 +29,8 @@ void checkEnhancementChangelog(in ref PullRequest pr, ref UserMessage[] msgs, if (!diff.canFind("\n+++ b/changelog/")) { msgs ~= UserMessage(UserMessage.Type.Warning, - "Pull requests implementing enhancements should include a [full changelog entry](https://github.com/dlang/dmd/tree/master/changelog)." + "Pull requests implementing enhancements should include a [full changelog entry](%s)." + .format(manualChangelogURL(pr.repoSlug)) ); } }