SFDX GitHub Code Review – Part 3

TL; DR;

Finally making the automated SFDX Pull Request Code Review useful by handling unlimited comments without API Rate Limiting issues, no comment duplication and mainly support for the fancy new Salesforce Graph Engine.

A few months back the result of this series was tagged in a GitHub conversation about SFDX code scanner. I was chuffed and determined to finally make it more complete. Over the holidays I finally started working on it and with my recent discovery of a little link with my name on it here I’ve got the energy to finish it.

What’s Changed

I’ve made sure the Salesforce Graph Engine is supported as that’s where the main drive came from. Then the open problem from last time was only being able to handle 40 ish comments. Current version can handle as many comments as are found. And to make that complete it also makes attempts to not duplicate comments it’s already made when it runs multiple times. Finally I’ve added input parameters for (almost) all the configuring arguments of the analyser plugin itself.

Check out the latest version here. I’d love for you to try it and help me smooth out any potential problems. I’m sure there’ll be a few. It’s mostly a late-hours effort after all.

I recently also added support to filter out issues less severe than a specified threshold. Thanks Adrian (see comments) for the suggestion.

For the dedicated readers a bit more about the how: 

Node

Some of the more complex logic was already done in javascript before. This is now greatly expanded and even all of the API calls to GitHub are made using the Octokit module. It’s still quite messy but I tried to extract at least parts of the code into modules. If I learned anything it’s that I really should go though some through formal js training. Luckily Mitch Spano’s similar GitHub Action was there to help provide some working examples.

const { Octokit } = require('@octokit/action');
 
module.exports = {
   createReview: async function (review) {
       const octokit = new Octokit();
 
       const {
           data: { id }
       } = await octokit.request('POST /repos/{owner}/{repo}/pulls/{pull_number}/reviews', {
           owner: review.owner,
           repo: review.repo,
           pull_number: review.pullNumber,
           body: review.body,
           comments: review.comments,
           event: review.event
       });
       return id;
   },

I still run the analyser outside of node and use the same python and jq hacks described in Part 1 of this series to transform the report. There was no reason to change this even though it could also easily all be done in node. But it ain’t broke so..

SFDX Analyzer Configuration Arguments

Previously only Category was supported. It was the most burning one for me as I needed to be able to at least suppress the Documentation rules (I don’t like comments). I am lazy so I didn’t want to add too much code. Luckily it’s possible to just pass empty strings as arguments into the CLI without the statement getting corrupted. 

run: sfdx scanner:run --target "${{ steps.getdiff.outputs.difflist }}" --category "${{ inputs.category }}" --engine "${{ inputs.engine }}" --eslintconfig "${{ inputs.eslintconfig }}" --pmdconfig "${{ inputs.pmdconfig }}" --tsconfig "${{ inputs.tsconfig }}" --format csv --normalize-severity  > report.csv

API Limitations

It’s questionable whether any more than the previous version’s maximum of 40 comments are beneficial. It just means the PR is way (way way way) too big and/or no care at all has been taken before creating it. Still, it should work.

GitHub API imposes some restrictions on how many requests you can submit (and how quickly). This is especially strict for requests that generate user notifications, like creating a Review or Comment. I went back and forth when making this work but ultimately ended up with the same from last time, 40 comments as part of the main review, and then further individual comments up to a configurable maximum in safe 5 second intervals. 

let sortedComments = comments.sort(filteredIssues, absoluteMaxComments);
prReview.comments = sortedComments.slice(0, PR_MAX_SIZE);
sortedComments = sortedComments.slice(PR_MAX_SIZE);
const reviewId = await github.createReview(prReview);
prReview.id = reviewId;

const { execSync } = require('child_process');
for (const issue of sortedComments) {
    let commentId = await github.addComment(prReview, issue);
    console.log(`Comment id: ${commentId} now waiting 5 seconds..`);
    execSync('sleep 5'); // block process for 5 seconds.
}

The original 40 is what I observed to always submit successfully without timing out. All the other subsequent comments will appear separately in the PR Conversation view. This doesn’t look amazing, but when they are needed the PR has other worse issues. I’d still recommend to limit the number of comments to a fairly small number. The current version of the action will take the most severe issues first and consider all issues found for the final approve/reject decision, even if not posting them all.

Comment Duplications

Another paint-point in the old version was re-running the review on a PR. It would create all the still persisting issues again, resulting in many duplications. That’s super annoying especially if you wanted to use it to actually finally approve the PR. 

That’s why now before creating any Review or Comments the action will retrieve all existing Reviews created by the github-actions[bot] user (separate individual comments are actually their own reviews). With some simple position-file-body key mapping the still persisting issues are not created again.

I wanted to also resolve the comments that are no longer an issue, but didn’t find a way to do that via GitHub API. Unless the reason is changed rules though, the comment will be shown as “obsolete” anyway. 

Salesforce Graph Engine

Seems like this should be the big one. But it wasn’t actually that hard. All I had to do was add support for the relevant arguments in the action input, add some conditional steps to run analyser normally or as dfa (or both) and update the script to read in 2 files of issues.

- name: 'Run Code Scanner'
    if: ${{ inputs.dfa_setting != 'dfa-only' }}
    shell: bash
    run: sfdx scanner:run --target "${{ steps.getdiff.outputs.difflist }}" --category "${{ inputs.category }}" --engine "${{ inputs.engine }}" --eslintconfig "${{ inputs.eslintconfig }}" --pmdconfig "${{ inputs.pmdconfig }}" --tsconfig "${{ inputs.tsconfig }}" --format csv --normalize-severity  > report.csv
    
- name: 'Run Graph Engine'
    if: ${{ inputs.dfa_setting != '' }
    shell: bash
    run: sfdx scanner:run:dfa --target "${{ steps.getdiff.outputs.difflist }}" --projectdir "${{ inputs.projectdir }}" --rule-thread-count "${{ inputs.rule_thread_count }}" --rule-thread-timeout "${{ inputs.rule_thread_timeout }}" --format csv --normalize-severity  > dfa-report.csv

There are some extra fields in the report because of the nature of the analysis: source file and sink file. The source (where path starts) is used to decide where the comment will be placed in the PR Diff View and the sink info (where the issue lies) is added to the body of the comment.

The Graph Engine can run for a very long time and need quite a lot of memory if your repo is large. So be careful when using this. Good thing this is intended for a PR review and only the changed files are analysed as entry points. This reduces the scope a bit making it usually run fine.

What’s Next

The comment contents would benefit from nicer formatting. I’ve also noticed that line number can now be used instead of the complicated position to place comments inline of the PR. I could ditch some of the code then. Generally the javascript code could use a lot of cleanup. 

I was also thinking about setting the same SFDX Code Review on Bitbucket and GitLab. But I think I might take a break from this for a while again. Unless someone convinces me otherwise again 🙂

Where to next

Bear Trap Alert

Queueable Running User in Unit Test

Mar 15, 20234 min read

I noticed that the Running User context of a Queueable Job in a unit test depends on who calls the Test.stopTest()…

ApexArchitectureTesting

DML (Mock) Service

Apr 6, 20226 min read

TL; DR; Apex utility wrapping DML operations and logging their results consistently. And a mock version of the same allowing…