Analysis is running on the whole code base not only the changes in the PR

mabumusa1 · June 13, 2023, 3:16pm

Hello there,
I am writing a Github action to analyze the code GitHub - mautic/mautic: Mautic: Open Source Marketing Automation Software., I managed to make it analyze PRs from forks. I also defined the the variables to make it scan the files changed in the PR itself but it is not picking that up.

Here is the code for github actions

github.com

mautic/mautic/blob/5.x/.github/workflows/sonar.yml

name: Sonar
on:
  workflow_run:
    workflows: [Mautic tests and validations]
    types: [completed]
jobs:
  sonar:
    name: Sonar
    runs-on: ubuntu-latest
    if: github.event.workflow_run.conclusion == 'success'
    steps:
      - uses: actions/checkout@v3
        with:
          repository: ${{ github.event.workflow_run.head_repository.full_name }}
          ref: ${{ github.event.workflow_run.pull_requests[0].head.sha }}
          fetch-depth: 0
      - name: 'Download code coverage'
        uses: actions/github-script@v6
        with:
          script: |

This file has been truncated. show original

And here is an example of the run

For reference

args: -Dsonar.organization=mautic -Dsonar.projectKey=mautic_mautic -Dsonar.links.homepage=https://github.com/mautic/mautic -Dsonar.links.ci=https://github.com/mautic/mautic/actions/workflows/tests.yml -Dsonar.links.scm=https://github.com/mautic/mautic -Dsonar.links.issue=https://github.com/mautic/mautic/issues -Dsonar.sourceEncoding=UTF-8 -Dsonar.sources=app,plugins -Dsonar.projectBaseDir=. -Dsonar.inclusions=app/*.php,app/**/*.php,plugins/*.php,plugins/**/*.php -Dsonar.exclusions=app/migrations/**/*,app/bundles/*Bundle/Config/**/*,app/bundles/*Bundle/DataFixtures/**/*,app/bundles/*Bundle/Tests/**/*,app/bundles/*Bundle/Translations/**/*,app/bundles/*Bundle/Views/**/*,app/middlewares/Test/**/*,app/bundles/CoreBundle/Test/**/*,plugins/*Bundle/Config/**/*,plugins/*Bundle/Tests/**/*,plugins/*Bundle/Translations/**/*,plugins/*Bundle/Views/**/* -Dsonar.cpd.exclusions=**/* -Dsonar.tests=tests -Dsonar.php.coverage.reportPaths=coverage/coverage.xml -Dsonar.php.tests.reportPath=coverage/junit.xml -Dsonar.scm.disabled=true -Dsonar.verbose=true -Dsonar.scm.revision=2c160f2af9754f49a1bf3551cc34ef31c5af1d8e -Dsonar.pullrequest.key=11613 -Dsonar.pullrequest.branch=add_mailer -Dsonar.pullrequest.base=5.x -Dsonar.newCode.referenceBranch=5.x

ganncamp · June 14, 2023, 5:51pm

Hi,

Welcome to the community!

It looks like you’re using v1.8 of the action, and v1.9.1 is the latest. An upgrade might help. Or just let it default as in this example?

Regarding the PR parameters, I believe they should be read from the environment and automatically filled in for you.

As a side note, I noticed this in your pipeline: -Dsonar.scm.disabled=true. Particularly if you’re trying to get accurate PR analysis, you really don’t want to disable the SCM data collection. Without SCM data, analysis can’t tell what’s new in your PR and will report ev-ry-thing.

HTH,
Ann

mabumusa1 · June 14, 2023, 8:14pm

Hi,

Thanks for your answer, I made some changes to the action description it now uses master version for the action, I also turned -Dsonar.scm.disabled=false.

Regarding, the PR parameters they are not picked up automatically because the come from external fork

I will merge and let you know the results

mabumusa1 · June 15, 2023, 10:55am

Thanks @ganncamp ,

I made the changes you suggested in the same Github workflow linked above, but unfortunately it did not pick up the changed lines, you can see the run log here

I do not know what is still missing?

ganncamp · June 15, 2023, 3:50pm

Hi,

In the most recent log (BTW, DEBUG logging is a bit much) I was eventually able to find this:

2023-06-15T13:14:41.6215381Z 13:14:41.534 DEBUG: SCM reported changed lines for 0 files in the branch
2023-06-15T13:14:41.6215527Z 13:14:41.534 INFO: SCM writing changed lines (done) | time=48ms

What code files are changed in this branch/PR?

Ann

mabumusa1 · June 15, 2023, 6:08pm

Thanks @ganncamp for your time.

Yes, all the PR got analyzed has changed files but the scan does not understand there is a change.

Here are some of the PRs

https://github.com/mautic/mautic/pull/12459
Fixing migrations by escopecz · Pull Request #12491 · mautic/mautic · GitHub

Would be great if we can get sonar working for Mautic, the community is getting bigger and developers would appreciate the analysis

Thank you for your help

ganncamp · June 16, 2023, 1:10pm

Hi,

Thanks for the pointer to the PR. I’m having a hard time finding the logs of its SonarCloud analysis. Could you point me to them? And to the PR on SonarCloud?

Thx,
Ann

mabumusa1 · June 16, 2023, 1:26pm

Hi @ganncamp ,

Okay, to remove the confusion let’s just study this PR in specific

github.com/mautic/mautic

Add Symfony Mailer Only

mautic:5.x ← mabumusa1:add_mailer

opened 04:37AM - 20 Oct 22 UTC

mabumusa1

+2245 -14477

| Q | A | -------------------------------------- | --- | Bug fix? (use the a.b branch) | [ ] | New feature/enhancement? (use the a.x branch) | 5.x | Deprecations? | Yes | BC breaks? (use the c.x branch) | BC breaks | Automated tests included? | Some automated tests are included, we need to add more to cover all the changes | Related user documentation PR URL | | Related developer documentation PR URL | [mautic/developer-documentation#... ](https://mautic-developer.readthedocs.io/en/5.x/components/emails.html#testing-email-transports) | Issue(s) addressed | Fixes #...  #### Description: **Email Bundle** In the `EmailBundle` we rely on `[Symfony/Mailer](https://symfony.com/doc/current/mailer.html)` and removed all the references to swiftmailer **What has changed ?** 1. All the transports have been removed because we want them to be plugins outside the core of Mautic core 2. Mailer is integrated with messenger so emails are queued directly to the database (or your choice of queue). 3. Callbacks classes remained the same but their URLs will be changed depending on the transport name for example `amazon_api` has been changed to `ses+api`. 4. There is a single parameter that configures mailer in a format of DSN like this `ses+api:username:password@default?region=us-east-1` 5. **Support for File Spool and File queue has been dropped**, in order to restore the queue functionality PR #11598 should be merged and then queues will be used **What is missing for this bundle** 1. Automated tests 2. Including other transports as plugins 3. Manual testing 4. Plugins to include all the transports that Mautic has now **This PR introduces the following improvements:** 1. Faster email sending, I tested with SES API and managed to get 3.5X speed increase compared to what we have in M4. 2. Less memory consumption as messenger compacts the emails when they are stored in the queue 3. Storage optimization, with Mautic 4 file spool, the disk may get filled with messages pending but with this change the messages will be loaded to Redis or the database and multiple processes can send at the same time. 4. We were able to run 32 processes to send emails simultaneously 6. This PR will be the door to introducing faster and more lightweight transports 7. We can use the transports that are maintained by Symfony without extra work on our side #### Steps to test this PR:  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs [here](https://contribute.mautic.org/contributing-to-mautic/tester)) 2. Try installing Mautic from the UI and the CLI 3. Send Emails from every possible place (config menu, campaign, segment, form, etc) 4. Make sure it works 5. Tips on the tests: include CC, BCC, attachments, emails with Unicode characters in the To, Reply-to, CC, From, BCC 6. **The email from should not include Unicode characters as specified in https://symfony.com/doc/5.4/mailer.html#email-addresses**  #### Immediate issues to fix after Symfony 5 upgrade 1. Make a better serialization of MauticMessage, right now it uses PHP native serialization, we need to convert it to JSON which is compact. 2. Include a bus for emails 3. Here is a sample of AWS SES transport and it shows how it inject itself to the refactored code https://github.com/mabumusa1/ses-plugin

This PR changed 186 files, but the analysis did not show any difference.

Appreciate your input on that

ganncamp · June 16, 2023, 2:18pm

Hi,

Thanks for the very concrete example. You have a lot of files in this PR and a lot of exclusions, but the exclusions only eliminate a few of them, and with the logs and the SonarCloud URL the logs provide, I’m able to verify that SonarCloud is indeed not finding any changed files. I can’t see anything obvious here, so I’ve flagged this for more expert eyes.

Ann

mabumusa1 · June 16, 2023, 2:44pm

Hi,

Thank you, I think if you check the other steps in the run would be great, we are using a GitHub actions that gets the details of the PR include last commit sha.

So that might helpful during your debug

Thank you for your support

mabumusa1 · June 20, 2023, 8:49am

Hi @ganncamp ,

I disabled the workflow for now, if you get any feedback, please let me know we will implement it immediately

Thanks

Claire_Villard · June 23, 2023, 2:16pm

Hi @mabumusa1,

The 2 PRs linked in this thread come from external forks.
Sadly, analyzing external pull requests is not supported yet in SonarCloud.
I would suggest that you vote for this feature candidate to help us build the SonarCloud roadmap:
https://portal.productboard.com/sonarsource/1-sonarcloud/c/50-sonarcloud-analyzes-external-pull-request

I also spotted that in the logs:

13:04:43.974 DEBUG: Exception caught during execution of command '[/usr/bin/git, config, --system, --show-origin, --list, -z]' in '/usr/bin', return code '128', error message 'fatal: unable to read config file '/etc/gitconfig': No such file or directory

The SonarScanner relies on the JGit library to detect the diff. It means the Scanner needs to have access to the Git configuration, and have the target branch history to be able to compute the diff. It may explain why the changed files are not seen.

Hope that helps,
Claire