also resolve conversations when dismissing reviews #76

renefritze · 2024-03-19T15:59:41Z

Resolving conversations is not available through the Rest API so we need to use the GraphQL one. This is the first time using it for me.
The mutation query needs the token to have contents: write. I found no documentation for this. Basically had to bisect the range of all possible perms to find that.
I've made the missing permission not fail the action to allow a "soft" upgrade path. An annotation is added to the github actions run. Not sure if that's a good path, it's not super obvious where that message comes from. Could use a link back to actual README?

Example with full permission

Example with missing permission

oleg-derevenetz · 2024-03-19T16:33:28Z

The mutation query needs the token to have contents: write. I found no documentation for this. Basically had to bisect the range of all possible perms to find that.

Since this functionality requires additional permissions, I would suggest making it optional, enabled via a special action config parameter, and disabled by default. Not everyone needs it (for example, I don't).

renefritze · 2024-03-20T07:39:34Z

Sure, I'll try to do that. When the action config is on, but the query detects permission's missing, I'd turn that into a an error output and step failure then, right?

I'll also address the pylint hints.

renefritze · 2024-03-20T16:59:30Z

I think I have addressed everything

renefritze · 2024-03-21T07:30:57Z

I did another set of test PRs with the updated action.
No auto resolving
Auto resolving
Auto resolving w/o setting the permission with failure

platisd

Great job! 👍 💯 🥇

run_action.py

platisd · 2024-03-21T19:43:07Z

run_action.py

+    if response.status_code == 200:
+        if "errors" in response.json():
+            msg = response.json()["errors"][0]["message"]
+            if "Resource not accessible by integration" in msg:
+                print(
+                    "::error::Closing conversations requires `contents: write` permission."
+                )
+            else:
+                print(f"::error::Closing conversation query failed: {msg}")
+        else:
+            print("Conversation closed successfully.")
+            return
+    else:
+        print(f"::error::GraphQL request failed: {response.status_code}")
+    print(
+        "::error:: Failed to close conversation. See log for details and "
+        "https://github.com/platisd/clang-tidy-pr-comments/blob/master/README.md for help"
+    )
+    raise RuntimeError("Failed to close conversation.")


Can we please refactor this to decrease the "depth" of the function? E.g. it should look like this, with the opposite logic and multiple return statements. No nested ifs:

if response.status_code != 200: print(f"::error::GraphQL request failed: {response.status_code}") # ... raise RuntimeError("Failed to close conversation.") if "errors" not in response.json(): print("Conversation closed successfully.") return msg = response.json()["errors"][0]["message"] # ....

With the current flow we don't have to repeat the runtime error + print for the error cases in the response.status_code == 200 branch.
Do you want me to pull the print+raise into a local function, do the reorder and call that function where needed?

I am OK with both the local function and also with this small duplication. 👍

run_action.py

platisd · 2024-03-21T19:49:10Z

run_action.py

+    """Generator of unresolved conversation threads to close
+
+    Uses the GitHub GraphQL API to get conversation threads for the given PR.
+    Then filters for unresolved threads and those that have been created by the action.


Question: Where do we select those created by the action?

Fair point. I guess we select open conversations started by the github-action user: https://github.com/platisd/clang-tidy-pr-comments/pull/76/files#diff-9981508c066d4bcb876bb89f7092ac196ce743485c2782d4142e6eb5e7c34457R561-R563
which might select conversations not actually started by the action. There's no message prefix to check for those though, right? Do you have another idea how to make the selection tighter?

Maybe it's a good idea to exploit that every message starts with ⚠️ .... ⚠️?

Ah, yes. Maybe pull up that string to either module level "constant" or pass it around like the warning prefix? Just so the two places cannot drift?

Sounds like a plan 👍

done. Tried to be explicit with using the kw-arg version in existing code.

Maybe it's enough with the single_comment_prefix for now, but what about a regex that checks that the first line starts and finishes with :warning:?

Updated to use a regex

Co-authored-by: Dimitris Platis <platisd@users.noreply.github.com>

…ting them

renefritze · 2024-03-22T11:31:30Z

Tried a new round of check PRs. Looks like I borked the comment-warning bit as it's no longer auto-closing in the test. Will revist later.

renefritze · 2024-03-25T11:11:38Z

New round of PRs that check new beahviour:

PR that only got the the change request closed
PR with right perms that got conversation closed
PR with auto-close enabled, but perms missing

oleg-derevenetz · 2024-03-25T21:40:48Z

action_launcher.bash

@@ -28,4 +28,5 @@ cd "$recreated_repo_dir"
  --repository "$GITHUB_REPOSITORY" \
  --repository-root "$recreated_repo_dir" \
  --request-changes "$INPUT_REQUEST_CHANGES" \
-  --suggestions-per-comment "$INPUT_SUGGESTIONS_PER_COMMENT"
+  --suggestions-per-comment "$INPUT_SUGGESTIONS_PER_COMMENT" \
+  --auto-resolve-conversations "$INPUT_AUTO_RESOLVE_CONVERSATIONS" \


There's an extra backslash.

also resolve conversations when dismissing reviews

69ee333

renefritze added 4 commits March 20, 2024 17:27

make auto-closing conversations an action setting

6096a3e

pylint| apply hints in existing code

806376c

pylint| apply hints in new code

2869250

failure to close a conversation makes the action fail

d8d04b0

fix missing whitespace in error message

f80caec

platisd requested changes Mar 21, 2024

View reviewed changes

renefritze and others added 3 commits March 22, 2024 09:00

apply cli arg suggestion

45dd14a

Co-authored-by: Dimitris Platis <platisd@users.noreply.github.com>

re-order response check flow

d0df268

fix cli arg suggestion application gone wrong

b831af1

renefritze force-pushed the resolve_conversations branch from f533399 to b831af1 Compare March 22, 2024 08:57

renefritze added 2 commits March 22, 2024 12:16

to-be-resolved convos also need to start with the prefix used in crea…

a08fa33

…ting them

refactor response check flow

bdac58f

renefritze force-pushed the resolve_conversations branch from 454dea5 to bdac58f Compare March 25, 2024 08:42

renefritze added 2 commits March 25, 2024 09:56

refactor single comment detection

5a8bad9

fix not passing correct comment marker to dismissal

c7c8637

renefritze force-pushed the resolve_conversations branch from 670e507 to c7c8637 Compare March 25, 2024 10:56

split too long format string

db891fd

platisd approved these changes Mar 25, 2024

View reviewed changes

platisd merged commit 837ad80 into platisd:master Mar 25, 2024
3 checks passed

oleg-derevenetz suggested changes Mar 25, 2024

View reviewed changes

renefritze deleted the resolve_conversations branch March 26, 2024 07:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

also resolve conversations when dismissing reviews #76

also resolve conversations when dismissing reviews #76

renefritze commented Mar 19, 2024

oleg-derevenetz commented Mar 19, 2024

renefritze commented Mar 20, 2024

renefritze commented Mar 20, 2024

renefritze commented Mar 21, 2024 •

edited

Loading

platisd left a comment

platisd Mar 21, 2024 •

edited

Loading

renefritze Mar 22, 2024

platisd Mar 22, 2024

platisd Mar 21, 2024

renefritze Mar 22, 2024

platisd Mar 22, 2024

renefritze Mar 22, 2024

platisd Mar 22, 2024

renefritze Mar 22, 2024

platisd Mar 22, 2024

renefritze Mar 25, 2024

renefritze commented Mar 22, 2024

renefritze commented Mar 25, 2024

oleg-derevenetz Mar 25, 2024

also resolve conversations when dismissing reviews #76

also resolve conversations when dismissing reviews #76

Conversation

renefritze commented Mar 19, 2024

oleg-derevenetz commented Mar 19, 2024

renefritze commented Mar 20, 2024

renefritze commented Mar 20, 2024

renefritze commented Mar 21, 2024 • edited Loading

platisd left a comment

Choose a reason for hiding this comment

platisd Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renefritze commented Mar 22, 2024

renefritze commented Mar 25, 2024

Choose a reason for hiding this comment

renefritze commented Mar 21, 2024 •

edited

Loading

platisd Mar 21, 2024 •

edited

Loading