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

chore: improve the dashboard comment query #5016

Merged
merged 4 commits into from
Jan 14, 2025
Merged

Conversation

dougch
Copy link
Contributor

@dougch dougch commented Jan 9, 2025

Release Summary:

Resolved issues:

n/a

Description of changes:

Our original GitHub dashboard has a "No review" block that should actually be PRs that have no comment, update the query.

Call-outs:

The distinction between review and comment is a bit pedantic.

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? tested using the GitHub Query https://github.com/search?q=repo%3Aaws%2Fs2n-tls+is%3Aopen+is%3Apr+comments%3A0+-label%3Astatus%2Fstale+-label%3As2n-core&type=pullrequests

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Jan 9, 2025
@dougch dougch marked this pull request as ready for review January 9, 2025 23:33
@dougch dougch requested a review from jouho January 9, 2025 23:33
@dougch dougch requested a review from lrstewart January 10, 2025 23:06
Comment on lines 76 to 77
title: 'No comment (external) last 14 days'
issue_query: 'repo:aws/s2n-tls is:open is:pr comments:0 -label:status/stale -label:s2n-core created:>{{ date("-14 days") }}'
Copy link
Contributor

Choose a reason for hiding this comment

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

What IS the "issue_query" distinction between comments and reviews? By looking for comments, are we now missing whatever "review" was? Or is it an "all reviews are comments, but not all comments are reviews" kind of situation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What IS the "issue_query" distinction between comments and reviews? By looking for comments, are we now missing whatever "review" was? Or is it an "all reviews are comments, but not all comments are reviews" kind of situation?

In the PR history, the banner on the update says 'review' or 'comment',

Doing a few trial queries, I'm going to say review:none by itself doesn't work. comments:0 does appear to work on it's own, as does combining the two (?!?).

  1. Both search terms 3 results, all matching the search
  2. just comment:0 6 results, all appear the match the search
  3. just review:none 34 results, but not correct, e.g. tests: make integV2 locally runnable #5029 which has a review

Copy link
Contributor

Choose a reason for hiding this comment

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

From that, it seems like a "review" is approve (and probably "request changes"). Like #5025 shows up in your "comment:0" search because it just got approvals, no comments. So it seems to me like what we want is both search terms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From that, it seems like a "review" is approve (and probably "request changes"). Like #5025 shows up in your "comment:0" search because it just got approvals, no comments. So it seems to me like what we want is both search terms?

Agreed- let me update it.

@dougch dougch requested a review from lrstewart January 14, 2025 18:31
.github/workflows/dashboard.yml Outdated Show resolved Hide resolved
@dougch dougch enabled auto-merge January 14, 2025 21:36
@dougch dougch added this pull request to the merge queue Jan 14, 2025
Merged via the queue into aws:main with commit 183833a Jan 14, 2025
42 checks passed
@dougch dougch deleted the dashboard_comments branch January 14, 2025 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants