Codacy in openmrs-core PRs?

  1. I dont know if codacy supports the checkstyle suppressions where we can exclude this rule only for test classes (see). I am asking their support.

  2. The way I solved it for the radiology module, which is not ideal, is excluding the test directorys. This can be done in the codacy UI.

If I am given permission to change settings on codacy I can for now apply the 2. option.

Sorry guys, I’ve just noticed I’m actually added to openmrs project admins on codacy. I’ve added burke (by your openmrs e-mail) to the project. @burke, you should be able to set up the integration now with openmrs-bot. Please disable the one in the infrastructure project.

I just wanted to point out, that the warning is coming from PMD and not checkstyle. It should be suppressible by adding a ruleset.xml file, which codacy should find. See [0] & [1]

its a bit more complicated.

  1. we are using checkstyle, the rules are in the repo which we need to load into codacy (as codacy has an autoload feature but it only works when the file is in the root of the repo). also I see we have different rules for webapp which I dont think works in codacy.

  2. the issue you are having is because codacy doesnt take into account our checkstyle suppression rules which are in a separate file. The workaround (which is not ideal) I just applied until I know of some other solution is that I added the api/src/test and web/src/test folders in the codacy UI to the ignored folders so this will help with your and other PRs.

  3. currently we are having a bit of a mixup. @burke can you please remove the infrastructure user/account you created for the openmrs-bot (since this one currently has the github PR integration and uses other rules; and does not ignore the test directories).

@raff added us both to the openmrs account on codacy as admins so you should then be able to add the integration on the correct codacy account here https://www.codacy.com/app/openmrs/openmrs-core/dashboard then the PRs should be fine again

Why not simply drop Codacy and keep using Sonar?

its been already mentioned in the thread but in summary:

  • we need github PR integration to drastically improve code quality of openmrs core and to then keep quality
  • we should have consistent code style/rules across modules
  • openmrs infrastructure is already in pain so even if we use the sonar github plugin for openmrs core, modules will not be able to use the same setup

I dont mind which tool we use but we need github PR integration

I doubt that Sonar is the culprit of the infrastructure problems (at least JIRA was crashing because out of memory problems, and RAM is not expensive ).

Anyway, SonarQube is also available in the cloud, free for OS projects. And IMHO the UI is better than Codacy’s

I personally don’t mind if developers prefer Sonar or Codacy, or even both.

But in all fairness, sonar is not causing any big harm to our infrastructure, indeed. If you tell me that we can have the same software for free in the cloud, with all the features we need, I’d totally prefer that :smiley:

good to know, thanks @lluismf

To summarize

I think we agree on:

  • OpenMRS community would be better of not hosting static code analysis itself
  • we should add github PR integration for static code analysis on openmrs-core (later on other modules as well to establish a common style)

What we dont yet fully agree:

There is more work needed to establish the github PR integration on https://sonarqube.com as shown https://docs.sonarqube.org/display/PLUG/GitHub+Plugin at the end where they refer to https://github.com/SonarSource/sonarqube/blob/master/travis.sh thats why I was leaning towards codacy

A few comments why Codacy is better than Sonar in our case:

  1. Currently Sonar is setup only for openmrs-core master. The full analysis takes ~40 minutes. We can afford to run it only every 24 hours. Doing it every commit could easily eat up all our CI agents and we would wait for other builds way too long. See https://ci.openmrs.org/browse/SON-OPENMRSCOREMASTER/latest. There’s an incremental analysis option for PRs, which is faster, but it cannot be used for analysis of the master branch since it doesn’t persist results in Sonar. Codacy on the other hand can be run every commit and every pull request and we get results in a few minutes.
  2. Given the above it is unrealistic to enable Sonar for our modules. Enabling Codacy for modules is no brainer and doesn’t require any additional configuration other than adding a project to Codacy.
  3. Sonar analysis even when using the cloud service is done on our CI (by maven sonar plugin). Basically the cloud service is just for storing/presenting results and setting up rules. Codacy analysis is done entirely on their infrastructure.
1 Like

I just got an answer from codacy’s support.

And identified following issues+solutions

  1. Codacy supports automatic loading of rules files (be it checkstyle, eslint, …) from the ROOT of the repository. We currently have 2 checkstyle files one for openmrs and one for webapp. Codacy expects it to be called checkstyle.xml and only loads one from the root directory. Please review my suggested solution so I can implement this: https://issues.openmrs.org/browse/TRUNK-5080 which will sync codacy’s rules with our checktsyle.xml

  2. Codacy does not support the checkstyle suppression filter which uses the checkstyle-suppressions.xml. I also checked https://docs.codeclimate.com/docs/checkstyle which doesnt state anything that suggest they do support this. I asked their support and am waiting. This feature as far as I see is only possible with SonarQube since it collects the report from the maven checkstyle plugin where this suppression filter is configurable. Current solution: codacy (in their UI) is configured to ignore test classes entirely.

  3. I added another task to update our maven checkstyle plugin which is very outdated: https://issues.openmrs.org/browse/TRUNK-5081 as part of this we could update our checkstyle.xml rules if anyone has suggestions.

@burke can you please remove the integration you added at https://www.codacy.com/app/infrastructure/openmrs-core/dashboard I now have permission to add the integration at https://www.codacy.com/app/openmrs/openmrs-core/dashboard

If we are not happy with codacy we can still switch to another provider later or stick with SonarQube since they are (after #1 is done) all based on the same checkstyle.xml rules, at any rate we should clean this up :sunny:

For me the most important set of rules is FindBugs (CheckStyle and PMD are less interesting). Please check that Codacy supports it.

What do you mean by for me? Does openmrs-core use PMD?

You can see all supported engines here (PMD is included, FindBugs only for enterprise)

https://support.codacy.com/hc/en-us/articles/213632009-Engines

Done.  

thank you! I just added the github integration for https://www.codacy.com/app/openmrs/openmrs-core/dashboard commits & PRs on the master branch should now be checked and commented

Excuse my english, I meant “IMHO the most important …” In order of usefulness: FindBugs > PMD > CheckStyle

ahh, no need to excuse :slight_smile: thanks!

I dont mind using PMD (FindBugs is currently not supported by codacy or codeclimate) I think you would just need to explain the benefit of it.

My focus is to get the rules enforced in our workflow and therefore clean this up:

Once we really use and adhere to the checkstyle rules (import one checkstyle.xml into codacy) we can also tweak them or switch to another rules engine like PMD. I mean the rules which were defined 6 years ago so they might benefit from changes too. But at first I would really like to finalize the automatic loading of the rules in codacy.

Check accepted answer http://stackoverflow.com/questions/4297014/what-are-the-differences-between-pmd-and-findbugs

I will add that Findbugs finds NPEs that will be surely thrown, and also possible NPEs (fa variable is initialized to null and some execution path does not give it a value).

That’s why they are complementary, PMD does things that Findbugs does not and vice-versa.

Just judging from that post I would say that we would benefit from adding FindBugs to our checks and that checkstyle does what PMD does (of course a more in depth comparison between checkstyle and PMD might show that one is better than the other).

Codacy enterprise supports FindBugs, but its not supported for open source projects.

Anyway I think we should simply start with checkstyle rules synced to codacy https://issues.openmrs.org/browse/TRUNK-5080

You can create a ticket for FindBugs with your thoughts on how this can be integrated.

I think I found a solution :slight_smile:

I was able to inline the location of the checkstyle-suppressions.xml into the checkstyle.xml

Can someone please have a look at

Not sure if anyone did that but you can run the checks locally with

mvn checkstyle:checkstyle

which will create checkstyle-result.xml files in the target directories.

The important aspect: PR brings a single checkstyle.xml in the root with a link to the checkstyle-suppressions.xml (which now exempts test classes from the rule preventing underscores in method names)

If you think thats a good solution, please merge :smile: and then I’ll import the checkstyle.xml into codacy.