website icon indicating copy to clipboard operation
website copied to clipboard

Update PR template with instructions regarding CodeQL annotations

Open roslynwythe opened this issue 2 years ago • 17 comments

Overview

We need to direct developers to check the PR for annotations resulting from CodeQL (Security and Code Quality) scanning.

Action Items

  • [x] Open the file .github/pull_request_template.md in your IDE
  • [x] Following the section "Why did you make the changes (we will use this info to test)?" add this section:
<h3>CodeQL Alerts</h3>


After the PR has been submitted and the resulting GitHub actions/checks have been completed, developers should check the PR for CodeQL alert annotations. 

If your issue has a CodeQL alert and is complexity: medium or higher, please let us know that you have checked and resolved.  Please do not dismiss alerts.
- [ ] I have checked this PR for CodeQL alerts.  If CodeQL alerts were found:
   - [ ] I have resolved CodeQL alerts 
   - [ ] I believe this CodeQL alerts is a false positive (merge team will evaluate)
   - [ ] I am stuck (after reading instructions below)

<details><summary>Instructions</summary>

If CodeQL alert/annotations appear, refer to [How to Resolve CodeQL alerts](https://github.com/hackforla/website/issues/6463#issuecomment-2002573270).  

In general, CodeQL alerts should be resolved prior to PR reviews and merging

</details> 


roslynwythe avatar Aug 14 '23 08:08 roslynwythe

Hi @roslynwythe.

Please don't forget to add the proper labels to this issue. Currently, the labels for the following are missing: Complexity, Role, Feature

NOTE: Please ignore the adding proper labels comment if you do not have 'write' access to this directory.

To add a label, take a look at Github's documentation here.

Also, don't forget to remove the "missing labels" afterwards. To remove a label, the process is similar to adding a label, but you select a currently added label to remove it.

After the proper labels are added, the merge team will review the issue and add a "Ready for Prioritization" label once it is ready for prioritization.

Additional Resources:

github-actions[bot] avatar Aug 14 '23 08:08 github-actions[bot]

I tried out the template... it seems like it might make a mess out of the PR if its not needed (because of the empty sections, which Markdown starts formatting weirdly). Let's brainstorm how to revise what we are adding.

ExperimentsInHonesty avatar Mar 04 '24 03:03 ExperimentsInHonesty

Investigate if its possible to trigger an GitHub comment if with the above template if the PR has a CodeQL alert.
If its feasable then have it post the template and any instructions

If not, figure out if we are going to move the info to a wiki page, and just have a check box on the PR that says does this issue have a CodeQL alert (don't check until the PR has been submitted). If yes, then follow these instructions and annotate your issue.

ExperimentsInHonesty avatar Mar 05 '24 01:03 ExperimentsInHonesty

Hi @t-will-gillis, thank you for taking up this issue! Hfla appreciates you :)

Do let fellow developers know about your:- i. Availability: (When are you available to work on the issue/answer questions other programmers might have about your issue?) ii. ETA: (When do you expect this issue to be completed?)

You're awesome!

P.S. - You may not take up another issue until this issue gets merged (or closed). Thanks again :)

github-actions[bot] avatar Mar 05 '24 01:03 github-actions[bot]

Reviewing how CodeQL currently handles notifications by forcing an error.

t-will-gillis avatar Mar 25 '24 22:03 t-will-gillis

I have been experimenting with creating errors on PR submissions to see how CodeQL functions:

  • From my tests, and from past PRs I have submitted (below), CodeQL does not always find and create notifications for problems until after a PR has been merged.
  • When CodeQL finds a problem during the PR review:
    • it is posting a message in the Conversation (see below).
    • I have not worked out -yet- how to trigger the bot to post another comment below the CodeQL message. I feel like this could be done eventually but I also feel like this is redundant i.e. posting a comment to say "pay attention to the above warning". Our comment warning about CodeQL won't be appearing without the CodeQL warning appearing first...
  • My vote is for the checkbox ("Check here to confirm that you have addressed any alerts" or something) and not posting the comment.

Reference:

  • PR 624 in my repo; corresponding Alert 131. If you review the resolved alerts, sometimes CodeQL found a problem, sometimes it did not.
  • PR #6011 and corresponding Alert 91

t-will-gillis avatar Apr 20 '24 23:04 t-will-gillis

@ExperimentsInHonesty @t-will-gillis

We do have wiki content (a draft comment) for How to Resolve CodeQL alerts

### Important: 
After creating a PR, developers should wait until GitHub checks are complete and then look for CodeQL annotations/alerts and if found, follow [How to Resolve CodeQL alerts](https://github.com/hackforla/website/issues/6463#issuecomment-2002573270) :

- [ ] Check this box indicate that this PR has been checked for CodeQL alerts.  Do not dismiss CodeQL alerts.  If changes were made in order to resolve the alert, describe those below.  If you are unable to resolve the alert, provide details below.  

### For Merge Team
- [ ] Do not merge a pull request with dismissed code alerts.  

Question: Since now we have a GHA to automatically create new issues to manage CodeQL alerts that are detected in the weekly scan, should we permit some PRs to be merged without resolving low severity alerts? Perhaps for smaller size issues?

roslynwythe avatar Jun 17 '24 00:06 roslynwythe

CodeQL Alerts

After the PR has been submitted and the resulting GitHub actions/checks have been completed, developers should check the PR for CodeQL alert annotations. If CodeQL alert/annotations appear, refer to How to Resolve CodeQL alerts.

?? In general, CodeQL alerts should be resolved prior to PR reviews and merging, but there is an exception for good first issue and Complexity: Small issues

  • [ ] I have checked this PR for CodeQL alerts. If CodeQL alerts were found:
    • [ ] I have resolved CodeQL alerts

roslynwythe avatar Sep 16 '24 18:09 roslynwythe

CodeQL Alerts

After the PR has been submitted and the resulting GitHub actions/checks have been completed, developers should check the PR for CodeQL alert annotations.

If your issue has a CodeQL alert and is complexity: medium or higher, please let us know that you have checked and resolved. Please do not dismiss alerts.

  • [ ] I have checked this PR for CodeQL alerts. If CodeQL alerts were found:
    • [ ] I have resolved CodeQL alerts
    • [ ] I believe this CodeQL alerts is a false positive (merge team will evaluate)
    • [ ] I am stuck (after reading instructions below)
Instructions

If CodeQL alert/annotations appear, refer to How to Resolve CodeQL alerts.

In general, CodeQL alerts should be resolved prior to PR reviews and merging

ExperimentsInHonesty avatar Sep 17 '24 00:09 ExperimentsInHonesty

Hello @jchue, we appreciate you taking on issue #5196, however it looks like you've already worked on enough issues of this complexity. Try checking out some of the issues of the next complexity from the Prioritized Backlog :)

We are going to unassign you from this issue so you can take on another issue.

Hfla appreciates you! :)

P.S. There is one exception to this rule/automation, and that is if you were away for a long time, and need to do the issue ladder again. If that is the case, please post the following note on the issue and on your Skills Issue (Pre-work Checklist). A Merge team member will reassign you to this issue, and will help you get assigned to subsequent issues up to medium size.

I am returning after a significant time away, and need to do the issue ladder again. Please assign me back to this issue.

HackforLABot avatar Sep 20 '24 23:09 HackforLABot

Hello @izma-mujeeb, we appreciate you taking on issue #5196, however it looks like you've already worked on enough issues of this complexity. Try checking out some of the issues of the next complexity from the Prioritized Backlog :)

We are going to unassign you from this issue so you can take on another issue.

Hfla appreciates you! :)

P.S. There is one exception to this rule/automation, and that is if you were away for a long time, and need to do the issue ladder again. If that is the case, please post the following note on the issue and on your Skills Issue (Pre-work Checklist). A Merge team member will reassign you to this issue, and will help you get assigned to subsequent issues up to medium size.

I am returning after a significant time away, and need to do the issue ladder again. Please assign me back to this issue.

HackforLABot avatar Sep 25 '24 02:09 HackforLABot

Hi @santisecco, thank you for taking up this issue! Hfla appreciates you :)

Do let fellow developers know about your:- i. Availability: (When are you available to work on the issue/answer questions other programmers might have about your issue?) ii. ETA: (When do you expect this issue to be completed?)

You're awesome!

P.S. - You may not take up another issue until this issue gets merged (or closed). Thanks again :)

HackforLABot avatar Sep 27 '24 14:09 HackforLABot

i. Availability: Friday-Sunday 9-27-24 ii. ETA: Saturday afternoon

santiseccovidal avatar Sep 27 '24 14:09 santiseccovidal

Changes have been made. However, I will ask in Sunday's meeting if I should test the PR template in my own repo before generating the pull request.

santiseccovidal avatar Sep 28 '24 21:09 santiseccovidal

I implemented the changes and opened a PR https://github.com/hackforla/website/pull/7546, but after some feedback on the changes I closed it because I would like to ask the following before reopening it.

I wanted to suggest adding below "After the PR has been submitted and the resulting GitHub actions/checks have been completed, developers should check the PR for CodeQL alert annotations." something similar to:

"CodeQL alert annotations can be checked below the submitted PR. You will see something similar to:

Click here to see a visual example of the `GitHub actions/checks` below the PR.

You can click on show all checks to see CodeQL checks

"

I believe this or something else will make it easier for new developers to understand where to find the Checks and CodeQL alert annotations. But just wanted to point that out because it will affect all devs creating PRs.

Should I leave it as it is or add that?

santiseccovidal avatar Oct 01 '24 18:10 santiseccovidal

@santisecco

Please add update using the below template (even if you have a pull request). Afterwards, remove the 'To Update !' label and add the 'Status: Updated' label.

  1. Progress: "What is the current status of your project? What have you completed and what is left to do?"
  2. Blockers: "Difficulties or errors encountered."
  3. Availability: "How much time will you have this week to work on this issue?"
  4. ETA: "When do you expect this issue to be completed?"
  5. Pictures (optional): "Add any pictures of the visual changes made to the site so far."

If you need help, be sure to either: 1) place your issue in the Questions/In Review column of the Project Board and ask for help at your next meeting, 2) put a "Status: Help Wanted" label on your issue and pull request, or 3) put up a request for assistance on the #hfla-site channel. Please note that including your questions in the issue comments- along with screenshots, if applicable- will help us to help you. Here and here are examples of well-formed questions.

You are receiving this comment because your last comment was before Tuesday, October 8, 2024 at 12:05 AM PST.

HackforLABot avatar Oct 11 '24 07:10 HackforLABot

I will address the concern tomorrow on the dev meeting.

santiseccovidal avatar Oct 12 '24 23:10 santiseccovidal