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.

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

[workflow] Don't add a comment when the first run of the formatter passes #86335

Merged
merged 3 commits into from
Mar 25, 2024

Conversation

tstellar
Copy link
Collaborator

@tstellar tstellar commented Mar 22, 2024

This was inadvertently changed in
2120f57.

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 22, 2024

@llvm/pr-subscribers-github-workflow

Author: Tom Stellard (tstellar)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/86335.diff

2 Files Affected:

  • (modified) .github/workflows/pr-code-format.yml (-1)
  • (modified) llvm/utils/git/code-format-helper.py (+2-1)
diff --git a/.github/workflows/pr-code-format.yml b/.github/workflows/pr-code-format.yml
index 2ed9b05cac120f..7adca02930237d 100644
--- a/.github/workflows/pr-code-format.yml
+++ b/.github/workflows/pr-code-format.yml
@@ -34,7 +34,6 @@ jobs:
         uses: actions/checkout@v4
         with:
           reository: ${{ github.repository }}
-          ref: ${{ github.base_ref }}
           sparse-checkout: |
             llvm/utils/git/requirements_formatting.txt
             llvm/utils/git/code-format-helper.py
diff --git a/llvm/utils/git/code-format-helper.py b/llvm/utils/git/code-format-helper.py
index af1bb3b5aec58d..f1207026704e88 100755
--- a/llvm/utils/git/code-format-helper.py
+++ b/llvm/utils/git/code-format-helper.py
@@ -124,7 +124,8 @@ def update_pr(self, comment_text: str, args: FormatArgs, create_new: bool) -> No
         existing_comment = self.find_comment(pr)
 
         if args.write_comment_to_file:
-            self.comment = {"body": comment_text}
+            if create_new or existing_comment:
+                self.comment = {"body": comment_text}
             if existing_comment:
                 self.comment["id"] = existing_comment.id
             return

@tstellar tstellar changed the title Code format comment fix [workflow] Don't add a comment when the first run of the formatter passes Mar 22, 2024
@tstellar
Copy link
Collaborator Author

This appears to work, but the workflow that adds the comment is now marked as failed since there is no artifact to download. The fix for this is to either have the code-formatter workflow produce an empty comment file or to have the pr-comment file to return success when there is no artifact. I'm thinking maybe the 2nd option is better.

@boomanaiden154
Copy link
Contributor

What are the advantages of the second option? Just producing an empty JSON file seems like the simplest solution to me.

@tstellar
Copy link
Collaborator Author

What are the advantages of the second option? Just producing an empty JSON file seems like the simplest solution to me.

It turns out there is no way to implement the 2nd option anyway, so I just went with the empty JSON.

Copy link
Contributor

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

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

LGTM, one comment.

@@ -34,7 +34,6 @@ jobs:
uses: actions/checkout@v4
with:
reository: ${{ github.repository }}
ref: ${{ github.base_ref }}
Copy link
Contributor

Choose a reason for hiding this comment

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

This was probably here for testing? Needs to be removed before pushing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I'll drop that commit.

Copy link

✅ With the latest revision this PR passed the Python code formatter.

Copy link

✅ With the latest revision this PR passed the C/C++ code formatter.

@tstellar tstellar merged commit de917dc into llvm:main Mar 25, 2024
4 checks passed
@jayfoad
Copy link
Contributor

jayfoad commented Mar 26, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants