mitchspano/sfdx-scan-pull-request

Error on "Run SFDX Scanner - Report finding as comments" step

vt89 opened this issue · 7 comments

vt89 commented

I am using the YAML available on README.md file but I am receiving an error at this step mitchspano/sfdx-scan-pull-request@v0.1.7:

2023-02-24T16:47:45.7640693Z     ErrorCaptureStackTrace(err);
2023-02-24T16:47:45.7640758Z     ^
2023-02-24T16:47:45.7640769Z 
2023-02-24T16:47:45.7640882Z <ref *1> Error: spawnSync /bin/sh ENOBUFS
2023-02-24T16:47:45.7641033Z     at Object.spawnSync (node:internal/child_process:1111:20)
2023-02-24T16:47:45.7641151Z     at spawnSync (node:child_process:814:24)
2023-02-24T16:47:45.7641257Z     at execSync (node:child_process:894:15)
2023-02-24T16:47:45.7641756Z     at getDiffInPullRequest (/home/runner/work/_actions/mitchspano/sfdx-scan-pull-request/v0.1.7/index.js:96:3)
2023-02-24T16:47:45.7642116Z     at main (/home/runner/work/_actions/mitchspano/sfdx-scan-pull-request/v0.1.7/index.js:325:3)
2023-02-24T16:47:45.7642444Z     at Object.<anonymous> (/home/runner/work/_actions/mitchspano/sfdx-scan-pull-request/v0.1.7/index.js:333:1)
2023-02-24T16:47:45.7642591Z     at Module._compile (node:internal/modules/cjs/loader:1105:14)
2023-02-24T16:47:45.7642754Z     at Object.Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
2023-02-24T16:47:45.7642895Z     at Module.load (node:internal/modules/cjs/loader:981:32)
2023-02-24T16:47:45.7643053Z     at Function.Module._load (node:internal/modules/cjs/loader:822:12) {
2023-02-24T16:47:45.7643151Z   errno: -105,
2023-02-24T16:47:45.7643261Z   code: 'ENOBUFS',
2023-02-24T16:47:45.7643398Z   syscall: 'spawnSync /bin/sh',
2023-02-24T16:47:45.7643506Z   path: '/bin/sh',
2023-02-24T16:47:45.7643587Z   spawnargs: [
2023-02-24T16:47:45.7643677Z     '-c',
2023-02-24T16:47:45.7643977Z     'git remote add -f destination MASKEDGIT'
2023-02-24T16:47:45.7644033Z   ],
2023-02-24T16:47:45.7644121Z   error: [Circular *1],
2023-02-24T16:47:45.7644203Z   status: null,
2023-02-24T16:47:45.7644312Z   signal: 'SIGTERM',
2023-02-24T16:47:45.7644382Z   output: [
2023-02-24T16:47:45.7644447Z     null,
2023-02-24T16:47:45.7644530Z     Buffer(21) [Uint8Array] [
2023-02-24T16:47:45.7644609Z        85, 112, 100,  97, 116, 105,
2023-02-24T16:47:45.7644689Z       110, 103,  32, 100, 101, 115,
2023-02-24T16:47:45.7644762Z       116, 105, 110,  97, 116, 105,
2023-02-24T16:47:45.7644832Z       111, 110,  10
2023-02-24T16:47:45.7644897Z     ],
2023-02-24T16:47:45.7644992Z     Buffer(1048576) [Uint8Array] [
2023-02-24T16:47:45.7645087Z        70, 114, 111, 109,  32, 104, 116, 116, 112, 115,  58,  47,
2023-02-24T16:47:45.7645188Z        47, 103, 105, 116, 104, 117,  98,  46,  99, 111, 109,  47,
2023-02-24T16:47:45.7645281Z       116,  45, 109, 111,  98, 105, 108, 101, 110, 101, 116, 104,
2023-02-24T16:47:45.7645368Z       101, 114, 108,  97, 110, 100, 115,  47,  84, 101, 115, 116,
2023-02-24T16:47:45.7645459Z        86,  84,  71, 105, 116,  82, 101, 112, 111,  10,  32,  42,
2023-02-24T16:47:45.7645548Z        32,  91, 110, 101, 119,  32,  98, 114,  97, 110,  99, 104,
2023-02-24T16:47:45.7645633Z        93,  32,  32,  32,  32,  32,  32,  32,  32,  32,  32,  32,
2023-02-24T16:47:45.7645718Z        32,  77,  65,  73,  78,  32,  32,  32,  32,  32,  32,  32,
2023-02-24T16:47:45.7645781Z        32,  32,  32,  32,
2023-02-24T16:47:45.7645862Z       ... 1048476 more items
2023-02-24T16:47:45.7645928Z     ]
2023-02-24T16:47:45.7645989Z   ],
2023-02-24T16:47:45.7646065Z   pid: 2006,
2023-02-24T16:47:45.7646161Z   stdout: Buffer(21) [Uint8Array] [
2023-02-24T16:47:45.7646232Z      85, 112, 100,  97, 116, 105,
2023-02-24T16:47:45.7646302Z     110, 103,  32, 100, 101, 115,
2023-02-24T16:47:45.7646374Z     116, 105, 110,  97, 116, 105,
2023-02-24T16:47:45.7646442Z     111, 110,  10
2023-02-24T16:47:45.7646502Z   ],
2023-02-24T16:47:45.7646606Z   stderr: Buffer(1048576) [Uint8Array] [
2023-02-24T16:47:45.7646711Z      70, 114, 111, 109,  32, 104, 116, 116, 112, 115,  58,  47,
2023-02-24T16:47:45.7646796Z      47, 103, 105, 116, 104, 117,  98,  46,  99, 111, 109,  47,
2023-02-24T16:47:45.7646884Z     116,  45, 109, 111,  98, 105, 108, 101, 110, 101, 116, 104,
2023-02-24T16:47:45.7646972Z     101, 114, 108,  97, 110, 100, 115,  47,  84, 101, 115, 116,
2023-02-24T16:47:45.7647058Z      86,  84,  71, 105, 116,  82, 101, 112, 111,  10,  32,  42,
2023-02-24T16:47:45.7647143Z      32,  91, 110, 101, 119,  32,  98, 114,  97, 110,  99, 104,
2023-02-24T16:47:45.7647229Z      93,  32,  32,  32,  32,  32,  32,  32,  32,  32,  32,  32,
2023-02-24T16:47:45.7647318Z      32,  77,  65,  73,  78,  32,  32,  32,  32,  32,  32,  32,
2023-02-24T16:47:45.7647390Z      32,  32,  32,  32,
2023-02-24T16:47:45.7647461Z     ... 1048476 more items
2023-02-24T16:47:45.7647581Z   ]
2023-02-24T16:47:45.7647651Z }

Any idea?

mitchspano commented

Thank you for raising this issue. I am unable to reproduce.
Are you able to share the exact YAML file and an outline of the repository structure?

image

mvcmania commented

Im receiving the same problem....
Here is the complete YAML.

# Unique name for this workflow
name: Pre-Release PR
# Definition when the workflow should run
on:
  workflow_dispatch:
  pull_request:
    types: [opened, edited, synchronize, reopened]
jobs:
  pmd-run:
    runs-on: catalyst
    steps:
      # Now we install nodejs in the VM, and specify version 14
      - uses: actions/setup-node@v3
        with:
          node-version: "14"
      # Install java as it is required for the next step
      - uses: actions/setup-java@v3
        with:
          distribution: "temurin"
          java-version: "8" # Always use Java v1.8 for building dependencies.
      # Checkout the source code
      - name: "Checkout source code"
        uses: actions/checkout@v3
        with:
          fetch-depth: 0
      # Now Install Salesforce CLI
      - name: Install SFDX CLI and Scanner
        run: |
          npm install sfdx-cli
          node_modules/sfdx-cli/bin/run plugins:install @salesforce/sfdx-scanner
      - name: Run SFDX Scanner - Report findings as comments
        id: scanner
        uses: mitchspano/sfdx-scan-pull-request@v0.1.7
        with:
          pmdconfig: ruleset.xml
          severity-threshold: 3
          strictly-enforced-rules: '[{ "engine": "pmd", "category": "Security,Best Practices,Performance"}]'
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
      #Check for PMD violations
      - name: "Fail if PMD violations"
        if: steps.scanner.outputs.violations != 0
        run: exit 1
mvcmania commented

I do believe the problem here is, specifying the severity-threshold parameter. The action throws hard exit when the particular threshold is met and the output message does not say the actual problem. I think the action should leave the continuity to consumer by providing some outputs.

mitchspano commented

@mvcmania What do you mean when you say "when the particular threshold is met and the output message does not say the actual problem"? As far as I am aware, there are no PMD violations which have an empty output message. Do you mean output message on the terminal?

I also don't think this is caused errors surpassing the error threshold. Below is an example which illustrates that findings surpassing the threshold do not produce the code: 'ENOBUFS' error described above.

Here is my sample YML:

name: Static Analysis
on:
  pull_request:
    types: [opened, reopened, synchronize]
  workflow_dispatch:
jobs:
  analyze:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
        with:
          fetch-depth: 0
      - name: Install SFDX CLI and Scanner
        run: |
          npm install sfdx-cli
          node_modules/sfdx-cli/bin/run plugins:install @salesforce/sfdx-scanner
      - name: Run SFDX Scanner - Report findings as comments
        uses: mitchspano/sfdx-scan-pull-request@v0.1.7
        with:
          severity-threshold: 1
          strictly-enforced-rules: '[{ "engine": "pmd", "category": "Performance", "rule": "AvoidDebugStatements" }]'
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

And a sample class added in the pull request:

global class SomeClass {
  public Boolean unusedVariable = false;
  public SomeClass() {
    System.debug('Oh no Mr Bill!');
  }

  private static void restrictedOperationInALoop() {
    for (Integer i = 0; i < 2; i++) {
      insert new Task();
    }
  }
}

Here is the output of the terminal in the action
image

Run mitchspano/sfdx-scan-pull-request@v0.1.7
Validating that this action was invoked from a pull request...
Getting difference within the pull request...
From https://github.com/mitchspano/testActions
 * [new branch]      main       -> destination/main
 * [new branch]      testPR     -> destination/testPR
Recursively moving all files to the temp folder...
Performing static code analysis on all of the files in the difference...
WARNING: We're continually improving Salesforce Code Analyzer. Tell us what you think! Give feedback at https://research.net/r/SalesforceCA
Getting existing comments using GitHub REST API...
Filtering the findings to just the lines which are part of the pull request...
Writing comments using GitHub REST API...
Error: A serious error has been identified

The findings are appropriately translated into comments and uploaded to the PR:

image

The Type column of the table in the comment's body will tell you if the job should be halted or not. Any finding with type of "Error" means the action will fail; "Warning" means the action will post the comment, but the action can continue successfully assuming no "Error" findings exist.

Users of this action can define how they want findings to be reported as "Error" vs "Warning" using two means:

  1. The severity-threshold parameter which will cause any findings past the threshold to be reported as errors.
  2. The strictly-enforced-rules parameter which can be used to enumerate any rules which if violated will be treated as "Error" regardless of their defined priority in the rules engine (PMD, Eslint, etc.)

In the example above, you will notice that the threshold is set to 1, and that both the Security.ApexSharingViolations as well as the Documentation.ApexDoc violations were reported as "Error" even though the only strictly enforced rule is pmd.Performance.AvoidDebugStatements.

Just for sake of completeness, I also updated the PR by removing the strictly-enforced-rules parameter and observed the same: both the Security.ApexSharingViolations as well as the Documentation.ApexDoc violations were reported as "Error", the findings were reported as comments, and the action's run was unsuccessful as we would expect.

@mvcmania I noticed this additional step in your yml file:

 #Check for PMD violations
      - name: "Fail if PMD violations"
        if: steps.scanner.outputs.violations != 0
        run: exit 1

This is not documented in the README.md. What made you think to include this in your action?

Also, your strictly-enforced-rules JSON is not formatted properly. In the documentation it states that the value should be:

"A JSON string which contains the rules which will be strictly enforced regardless of their priority. Enforced rules are identified by their engine, category, and rule name."

Your JSON string is:

'[{ "engine": "pmd", "category": "Security,Best Practices,Performance"}]'

Which doesn't specify any rule names, and groups a bunch of categories together which is not supported.

We would expect the JSON string to be of a similar format to this:

'[{ "engine": "pmd", "category": "Performance", "rule": "AvoidDebugStatements" }]'

If we want to change the JSON structure to permit category level strict enforcement, we would need to raise a new idea to track the work against.

mvcmania commented

hi @mitchspano, thanks for detailed answer. First off all, the your findings on my yaml file is just because of some test attempts, i m total aware of the strictly-enforced-rules: format , and i m also aware of that documentation does not mention about something below:


- name: "Fail if PMD violations"
        if: steps.scanner.outputs.violations != 0
        run: exit 1

Yes i believe the problem is severity-threshold parameter as you have already explained. I mean not the problem but it produces the output message on terminal.
And yes again, what i meant is terminal output message which is not quite explanatory. "Error: A serious error has been identified" it sounds like some unexpected error happened and the job is halted. Dont you think so ?
And yes you are right, all the findings, errors and warning are being added as comment perfectly, however my intent is to send a slack notification or take some actions if there are any serious findings in the PR found but your action has no straight way to determine whether there are errors or warning unless you may want to check the PR comments. That is why i have tried to capture the output of your action which were not available despite the fact that that is not in the documentation.
I hope that is clear now. Thank you again.

mitchspano commented

Okay thanks for the clarification
I just want to confirm that I understand correctly; are all of the following statements true?

  1. You are not experiencing the code: 'ENOBUFS' error explained in the original issue description.
  2. The action is correctly generating in-line comments with the findings produced from the scanner and halting whenever an finding of type "Error" is identified.
  3. The definition of "Error" as opposed to "Warning" via severity-threshold and strictly-enforced-rules is working as intended.
  4. The error message of A serious error has been identified in the terminal is not as clear as one might hope. Perhaps we could improve the fidelity of the error message to give greater clarity to the user about why the action failed.
  5. The fact that there are no defined output parameters leaves users without the ability to reference the findings in subsequent actions such as a Slack message.

I was confused because your first comment began with "Im receiving the same problem...." but it sounds like you are not experiencing the same code: 'ENOBUFS' error described by the issue's original author, and your example with steps.scanner.outputs.violations exiting with a non-zero exit code is already being performed via core.setFailed(errorMessage) whenever an "Error" finding is produced.

I think that improving the error message and adding output parameters are very valid feedback and would improve the quality of this action, but I don't think fall under the scope of this issue, so I have created #29 and #30 to log these as enhancements and track the work there.

mitchspano commented

@mvcmania and @vt89, in the v0.1.8 release, we have removed the passing of the severity-threshold from the yml input to the scanner CLI command. Now, we just use that when parsing the findings. This should resolve any issues caused by that parameter and its value.

Please let me know if you are still experiencing issues with this and we will re-open this issue.