Add PR review reminder DAG #553

sarayourfriend · 2022-06-09T17:13:59Z

Fixes

Fixes WordPress/openverse#1540 by @zackkrida

Description

Adds a new DAG to run daily and get PR review reminders.

Testing Instructions

Checkout this PR locally, add a GitHub PAT with access to the relevant repositories, and do a dry run to check if it will ping as expected.

Also check out the unit tests.

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

obulat · 2022-06-20T12:12:37Z

openverse_catalog/dags/common/pr_review_reminders.py

+        return Urgency("low", 5)
+
+
+def days_without_weekends(today: datetime, delta: timedelta) -> int:


It would be super cool if we could also get the AFK status here...

obulat · 2022-06-20T12:14:26Z

openverse_catalog/dags/maintenance/pr_review_reminders.py

+The DAG does not ping on Saturday and Sunday and accounts for weekend days
+when determining how much time has passed since the review.
+
+Unfortunately the DAG does not know when someone is on vacation. It is up to the


The answer to my comment above is here :)

obulat

This will be a great addition to our workflow!

AetherUnbound

This is awesome!! I'm very excited to have this as part of our workflow 🚀

I'm not sure how to test the other stuff as it all depends on GitHub responses. I could record some of them but it's a real pain without setting up infrastructure for that.

I think that's a solid idea, it's what we do for the provider API script tests: https://github.com/WordPress/openverse-catalog/tree/main/tests/dags/providers/provider_api_scripts/resources.

env.template

openverse_catalog/dags/common/github.py

openverse_catalog/dags/common/pr_review_reminders.py

openverse_catalog/dags/maintenance/pr_review_reminders.py

AetherUnbound · 2022-06-21T19:48:07Z

One more point I noticed while looking over this again - currently this is the structure:

common/
    github.py
    pr_review_reminders.py
maintenance/
    pr_review_reminders.py

The common directory is great for functions/classes/etc that we're likely to reuse, but I think it'd be best to keep operational code that's specific to a DAG as close to the DAG as possible. @stacimc and I discussed this a bit recently with the updated provider DAG factory. The following would probably work great, as it follows some other conventions we have:

common/
    github.py
maintenance/
    pr_review_reminders/    
        pr_review_reminders.py
        pr_review_reminders_dag.py  # or pr_review_reminders_workflow.py

What do you think? This puts the PR review specific code next to the DAG, since I think we're unlikely to reuse those functions in the same way we could reuse github.py.

sarayourfriend · 2022-06-23T00:10:35Z

Thanks for the thorough review @AetherUnbound. This should be ready for another one now.

sarayourfriend · 2022-06-23T02:35:45Z

openverse_catalog/dags/maintenance/pr_review_reminders/pr_review_reminders.py

+class Urgency:
+    @dataclass
+    class Urgency:
+        label: str
+        days: int
+
+    CRITICAL = Urgency("critical", 1)
+    HIGH = Urgency("high", 2)
+    MEDIUM = Urgency("medium", 4)
+    LOW = Urgency("low", 5)


It's kind of annoying that you can't reference a class within its own definition. I wish there was something like an enum that also allowed for complex, named values. This is probably the first time I've ever thought to myself "wow I really wish $x$ Java feature existed here" 😂

AetherUnbound

Looks great! These tests are super thorough 🚀 Just a couple of requests/questions, nothing to stop a merge.

docker-compose.override.yml

AetherUnbound · 2022-06-23T18:44:07Z

tests/dags/conftest.py

+    Now() manager patches datetime return a fixed, settable, value
+    (freezes time)
+
+    https://stackoverflow.com/a/28073449 CC BY-SA 3.0


Thank you for including this!

AetherUnbound · 2022-06-23T18:48:27Z

tests/factories/github/__init__.py

+_pr_count = 1
+
+
+def make_pull(priority: Urgency, past_due: bool) -> dict:


Do you mind adding a docstring here describing what this function does? I'm having a hard time following everything it's referencing.

AetherUnbound · 2022-06-23T18:48:58Z

tests/factories/github/__init__.py

+
+    for label in pull["labels"]:
+        if "priority" in label["name"]:
+            label.update(**_make_label(priority))


Why is this step necessary?

We use the priority label on the PR to determine the urgency of the PR, so we have to update the priority label in the pull.json fixture to have the configured urgency. This allows us to test all four urgency levels.

Ahh! So it's replacing whatever's in the fixture with what you're using in the test, makes sense 💯

[WIP] Add PR review reminder DAG

bc5e08d

openverse-bot added this to In progress in Openverse PRs Jun 9, 2022

openverse-bot added 🟧 priority: high Stalls work on the project or its dependents 🤖 aspect: dx Concerns developers' experience with the codebase 🧰 goal: internal improvement Improvement that benefits maintainers, not users labels Jun 9, 2022

sarayourfriend added 2 commits June 16, 2022 07:22

Use requests directly instead of HttpHook

3fa588e

Add unit tests and fix weekday calculation

8af4737

sarayourfriend changed the title ~~[WIP] Add PR review reminder DAG~~ Add PR review reminder DAG Jun 15, 2022

sarayourfriend marked this pull request as ready for review June 15, 2022 21:57

sarayourfriend requested a review from a team as a code owner June 15, 2022 21:57

sarayourfriend requested review from AetherUnbound and obulat June 15, 2022 21:57

openverse-bot moved this from In progress to Needs review in Openverse PRs Jun 15, 2022

obulat reviewed Jun 20, 2022

View reviewed changes

obulat approved these changes Jun 20, 2022

View reviewed changes

AetherUnbound reviewed Jun 20, 2022

View reviewed changes

sarayourfriend added 6 commits June 21, 2022 08:09

Use crontab config to only run on weekdays

449c254

Move repos into a constant

e87cd6f

Reduce calls to base_repo_name

537a755

Remove unnecessary template configuration

6ab61c7

Refine argument types

69f524a

Use session and rename var for masking

fa83993

sarayourfriend added 2 commits June 22, 2022 06:56

Move files and reduce necessary calls

8e5d584

Add unit tests and fix bugs

8acf39f

sarayourfriend requested a review from AetherUnbound June 23, 2022 00:08

Move files and clean up default dry_run values

f638ca7

Correct Urgency types

11f1160

sarayourfriend commented Jun 23, 2022

View reviewed changes

Remove unused function

c7e1b1b

AetherUnbound approved these changes Jun 23, 2022

View reviewed changes

Openverse PRs automation moved this from Needs review to Reviewer approved Jun 23, 2022

Add docs to test factory

f9b85da

sarayourfriend merged commit f5de3e2 into main Jun 23, 2022

Openverse PRs automation moved this from Reviewer approved to Merged! Jun 23, 2022

sarayourfriend deleted the add/pr-reviews-ping branch June 23, 2022 21:48

AetherUnbound mentioned this pull request Jun 23, 2022

Fix module import for PR review reminder DAG #566

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PR review reminder DAG #553

Add PR review reminder DAG #553

sarayourfriend commented Jun 9, 2022 •

edited

Loading

obulat Jun 20, 2022

obulat Jun 20, 2022

obulat left a comment

AetherUnbound left a comment

AetherUnbound commented Jun 21, 2022

sarayourfriend commented Jun 23, 2022

sarayourfriend Jun 23, 2022 •

edited

Loading

AetherUnbound left a comment

AetherUnbound Jun 23, 2022

AetherUnbound Jun 23, 2022

AetherUnbound Jun 23, 2022

sarayourfriend Jun 23, 2022

AetherUnbound Jun 23, 2022

		return Urgency("low", 5)


		def days_without_weekends(today: datetime, delta: timedelta) -> int:

		_pr_count = 1


		def make_pull(priority: Urgency, past_due: bool) -> dict:

Add PR review reminder DAG #553

Add PR review reminder DAG #553

Conversation

sarayourfriend commented Jun 9, 2022 • edited Loading

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

obulat Jun 20, 2022

Choose a reason for hiding this comment

obulat Jun 20, 2022

Choose a reason for hiding this comment

obulat left a comment

Choose a reason for hiding this comment

AetherUnbound left a comment

Choose a reason for hiding this comment

AetherUnbound commented Jun 21, 2022

sarayourfriend commented Jun 23, 2022

sarayourfriend Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

AetherUnbound left a comment

Choose a reason for hiding this comment

AetherUnbound Jun 23, 2022

Choose a reason for hiding this comment

AetherUnbound Jun 23, 2022

Choose a reason for hiding this comment

AetherUnbound Jun 23, 2022

Choose a reason for hiding this comment

sarayourfriend Jun 23, 2022

Choose a reason for hiding this comment

AetherUnbound Jun 23, 2022

Choose a reason for hiding this comment

sarayourfriend commented Jun 9, 2022 •

edited

Loading

sarayourfriend Jun 23, 2022 •

edited

Loading