ER: Create documentation for reviewing CodeQL PRs

[elliot-d-kim]

Emergent Requirement - Problem

When reviewing PRs that are supposed to resolve CodeQL alerts, there is a lack of documentation on how to verify that the code changes resolve the alert. This expends developer resources and may allow improper code reviews.

Issue you discovered this emergent requirement in

• Resolve codeql alert 51 6622 #6651
• Refactor codeql.yml file #6691
• Note: Encountered while reviewing PRs, not fixing an issue

Date discovered

4/19/24

Did you have to do something temporarily

• YES
• NO

Who was involved

@gaylem, @t-will-gillis, @marioantonini, @Dartis4

What happens if this is not addressed

1. Reviewers may spend much more time figuring out how to verify that CodeQL alerts are resolved (or are blocked entirely)
2. Senior contributors must dedicate more time and attention to helping confused contributors
3. Alternatively, reviewers approve code changes without properly testing

Resources

• Research and Provide a Demo of Unit Test Implementation for HfLA Repo #2249
  • @Dartis4 is exploring the possibility of overhauling hackforla's testing framework, which may resolve this issue as an aside. However, this appears to be far from implementation and deployment at this time.

Recommended Action Items

• Make a new issue
• Discuss with team
• Let a Team Lead know

Potential solutions [draft]

Verify that the code changes flip the target CodeQL alert from 'Open' to 'Fixed'

1. Capture CodeQL status on personal fork before code changes
   a. Navigate to personal fork remote repo
   b. From fork, create new branch according to following naming scheme: contributorUsername-issueBranchName. This is to match the name of the branch created later in Step 2a.
   c. Set issue branch as default: Settings > General > Default branch > Switch > select newly created issue branch
   d. Run workflow from branch: Actions > CodeQL Scan > Run workflow > Use workflow from 'Branch: [name of the branch]' > Run workflow
   e. Find target alert: Security > Code scanning > (use filters/details like filename to find the same alert)
   • Note: Alert numbers will not be the same across hackforla repo and personal fork

EG: Before code changes applied: note 'Open' label

2. Integrate proposed code changes into branch to verify CodeQL resolution after code changes
   a. Create branch locally and pull proposed code changes from PR (refer to commandline instructions in Step 3 of 'How to Review Pull Requests')
   b. Modify .github/workflow/codeql.yml: Under on: > push:, set branches: ['name of the branch']
   c. git add, commit, push. This should push the code changes to the branch that was previously created in Step 1, making the two branches one.
   d. At this point, a new workflow run should be queued to be run. Check under Actions > CodeQL Scan
   e. Once workflow finishes, check that the CodeQL alert has been closed: Security > Code scanning > Closed

EG: After code changes applied: note 'Fixed' label

Other common problems encountered when reviewing CodeQL PRs

Great documentation exists for the following, but contributors would benefit from this documentation being more obviously accessible

• Personal access tokens (Tip 7 from Hack for LA's GitHub Actions)
  • Not having PATs set up causes failed workflows that may be confusing to reviewers
• Project board: duplicate from hackforla repo (Tip 6 from Hack for LA's GitHub Actions)
  • Not having classic project board set up causes 'Issue Trigger' workflow to fail
  • Note: Classic project boards (like hackforla's) are being sunsetted and duplication is no longer possible until the project is migrated to the new Projects format.

[github-actions]

Hi @elliot-d-kim.

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 this 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:

• Wiki: How to create issues
• Wiki: How to read and interpret labels

[roslynwythe]

@elliot-d-kim you raised a good point in this ER. My assumption had been that if CodeQL did not raise any alerts/annotations in the Pull Request process, we could be assured that the updated code would resolve the alert. Do we understand why that would not be the case? I too have seen instances where CodeQL seems to have failed to flag an issue and I would like to understand more about why that happens. We do still have instances of liquid/YAML code in many files, which is not supported by CodeQL so I'm wondering if that might be the cause of CodQL odd behavior.

[ExperimentsInHonesty]

@elliot-d-kim - Roslyn left you a message on this issue #6904 (comment)

[HackforLABot]

Hi @elliot-d-kim, 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 :)