[openstreetmap/openstreetmap-website] Added workflow for PR labeling using Danger (PR #4988)

Andy Allan notifications at github.com
Wed Aug 21 14:56:44 UTC 2024


@gravitystorm requested changes on this pull request.

Overall I'm very in favour of this PR, and I only have a few minor requests to improve it.

> @@ -0,0 +1,44 @@
+# Get PR number and initialize is_good_pr variable
+pr_number = github.pr_json["number"]
+is_good_pr = true
+
+# Report if number of changed lines is > 500
+if git.lines_of_code > 500
+  warn("Number of updated lines of code is too large to be in one PR. Perhaps it should be separated into two or more?")
+  auto_label.set(pr_number, "Big PR", "ffff00")

These colours are a bit eye-catching for labels. Here's some colour values from the github default colour set, which will harmonise better with our existing labels:

#B60205 - red
#D93F0B - orange
#FBCA04 - yellow

However, I've also checked the source code for the danger-auto_label plugin, and the colour values are ignored if the label already exists. This means that we can change them in future without them being overridden by this code, so maybe it doesn't matter what we set them to here.

> +# Report "Everything is fine!" if no warnings were generated
+if is_good_pr
+  message("Everything is fine!")
+  auto_label.set(pr_number, "Good PR", "00ff00")
+else
+  auto_label.remove("Good PR")
+end

I don't think we need labels on good prs, but I'm open to feedback on this. If we keep the label, then the name of the label needs to be changed to be more accurate - it's only "Good" as far as Danger is concerned, and it will be very confusing to have "Good PR" added to a PR that fails CI, which is a much more thorough assessment of the PR.

Similarly, the success message (if any) should be something different, which reflects that some aspects of the PR have been checked, but without implying that "everything" is OK.



-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/4988#pullrequestreview-2251048328
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/4988/review/2251048328 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20240821/9d3b59a5/attachment-0001.htm>


More information about the rails-dev mailing list