Skip to content

Check if existing comment contains "Squawk Report" header. #477

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.

Sign up for GitHub

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

Merged
kodiakhq merged 2 commits into from
May 14, 2025
Merged

Check if existing comment contains "Squawk Report" header. #477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8b5f209
Check if existing comment contains "Squawk Report" header.
danxmoran May 14, 2025
8ac4642
Add comment
danxmoran May 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion crates/squawk/src/github.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ fn create_gh_app(
)
}

const COMMENT_HEADER: &str = "# Squawk Report";

pub fn check_and_comment_on_pr(
cmd: Command,
cfg: &Config,
Expand Down Expand Up @@ -130,6 +132,7 @@ pub fn check_and_comment_on_pr(
&github_repo_name,
github_pr_number,
&comment_body,
COMMENT_HEADER,
)?;

let violations: usize = file_results.iter().map(|f| f.violations.len()).sum();
Expand All @@ -149,7 +152,7 @@ fn get_comment_body(files: &[CheckReport], version: &str) -> String {

format!(
r"
# Squawk Report
{COMMENT_HEADER}

### **{violations_emoji} {violation_count}** violations across **{file_count}** file(s)

Expand Down
16 changes: 12 additions & 4 deletions crates/squawk_github/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,24 @@ pub fn comment_on_pr(
repo: &str,
issue: i64,
body: &str,
existing_comment_text_includes: &str,
) -> Result<(), GithubError> {
let comments = gh.list_issue_comments(owner, repo, issue)?;

let bot_name = gh.app_slug();

info!("checking for existing comment");
match comments
.iter()
.find(|x| x.user.r#type == "Bot" && x.user.login == bot_name)
{
match comments.iter().find(|x| {
x.user.r#type == "Bot"
&& x.user.login == bot_name
// NOTE: We filter comments by their contents so we don't accidentally
// overwrite a comment made by some other tool. This happens often in
// GitHub repos that reuse the default GHA bot for all linters.
//
// This only works if `existing_comment_text_includes` is a "stable"
// piece of text included in all comments made by squawk!
&& x.body.contains(existing_comment_text_includes)
Copy link
Contributor Author

@danxmoran danxmoran May 14, 2025

Choose a reason for hiding this comment

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

I thought about using starts_with here to be even more specific, but I wasn't sure how that would play with the markdown generation

Copy link
Owner

@sbdchd sbdchd May 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

ah is the issue that there's one bot account setup and multiple apps, other than squawk, use it?

Edit: read your issue, makes sense!

Copy link
Owner

@sbdchd sbdchd May 14, 2025

Choose a reason for hiding this comment

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

can we add a comment explaining the reasoning? otherwise looks good!

Copy link
Contributor Author

@danxmoran danxmoran May 14, 2025

Choose a reason for hiding this comment

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

Added!

}) {
Some(prev_comment) => {
info!("updating comment");
gh.update_issue_comment(owner, repo, prev_comment.id, body)
Expand Down
Loading