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 🙂
Thanks a lot for sharing the action, Aleš! It works great. One small mention, the comments still seem to get duplicated with the second run. Not sure if you are still planning to optimize it but even if not, thanks again for sharing your efforts with us. Adrian
Thanks very much Adrian. I’ll make sure to double check the duplications and update accordingly. Can’t promise much on the timeline though 🙂
Nice one, Aleš, the action is fantastic!
In case you will start working on it again in the future, if you allow me to write down in here another idea – Add comments based on the severity.
Example: As my project is currently a mess, I would firstly want to tackle the severity 1 & 2 issues, hence sev 1 & 2 comments in my developers’ pull-requests would be enough for a while (not ideal, I know)
Ofc, the variable name in the above example is just a dummy one.
I would love to be able to “ask” Salesforce Code Analyzer to only give me back the sev 1 & 2 (as example) but as far as I can see they do not have this filter yet. So not sure how much you can do without their help.
You are the best!
Adrian
That’s a pretty good suggestion, Adrian. I think I might add that soon.
You can already achieve this by specifying you own engine configurations. For example your own ruleset xml via the –pmdconfig parameter. It’s a good idea since you can have that and say the eslint config be part of your repo and use the same config both in the PR and locally when developing.
But yea, I can see why you might want to ignore these in the PR but still have them show up locally.
Hi Ales!
Coming back again after enjoying your action together with my team (I am working as a Release Manager, hence if any of my queries do not make much sense, that’s the reason, I have no apex development skills :D).
Any chance to have a new field, similar with Category, but for a more granular level, the rules level. In my case, my colleagues developers are stating that the rule in the image does not add to much value, hence can be skipped.
—————————————–
category: ‘!Documentation’
rule: ‘!ApexFlsViolationRule’
—————————————–
Alternatively, I can build the ruleset.xml file and use it as you recommended to me a while ago.
Thanks a lot,
Adrian
Hi Adrian,
this would definitely be a case for rulesets. It would just be re-inventing what’s already available. If your team does not wan to consider some of the rules in general.
I’m glad you’re finding my action useful