Hi Stamatis, glad to hear you find Sonar helpful, thanks for providing your feedback.
The master branch analysis already provides what I think you are looking for, you have: - all code analysis (to see the full status of the code): https://sonarcloud.io/summary/overall?id=apache_hive - new code analysis (basically what changed in the last commit): https://sonarcloud.io/summary/new_code?id=apache_hive For PRs, similarly, the analysis covers the changes w.r.t. the target branch, it's a good and quick way to ascertain the code quality of the PR. Regarding "Is it possible to somehow save the current analysis on master and make the PR quality gates fail when things become worse?", it is definitely possible, we can define a success/failure threshold for each of the metrics, and make it fail if the quality gate criteria are not met. I was suggesting to postpone this to allow people to get first familiar with it, I would not want to disrupt existing work, Sonar is a rich tool and people might need a bit of time to adjust to it. Good news is that quality gates can be changed directly from SonarCloud and won't require code changes, we might kick in a feedback discussion after a month or so from when we introduce Sonar analysis and see what people think. Best regards, Alessandro On Tue, 9 Aug 2022 at 16:38, Stamatis Zampetakis <zabe...@gmail.com> wrote: > Hi Alessandro, > > Sonar integration will definitely help in improving cope quality and > preventing bugs so many thanks for pushing this forward. > > I went over the PR and it is in good shape. I plan to merge it in the > following days unless someone objects. > We can tackle further improvements in follow up JIRAs. > > Is it possible to somehow save the current analysis on master and make the > PR quality gates fail when things become worse? > If not then what may help in reviewing PRs is to have a diff view (between > a PR and current master) so we can quickly tell if the PR we are about to > merge makes things better or worse; as far as I understand the idea is to > do this manually at the moment by checking the results on master and on the > PR under review. > > Enabling code coverage would be very helpful as well. Looking forward to > this. > > Best, > Stamatis > > On Mon, Aug 8, 2022 at 1:22 PM Alessandro Solimando < > alessandro.solima...@gmail.com> wrote: > > > Errata corrige: the right PR link is the following > > https://github.com/apache/hive/pull/3254 > > > > Best regards, > > Alessandro > > > > On Mon, 8 Aug 2022 at 10:04, Alessandro Solimando < > > alessandro.solima...@gmail.com> wrote: > > > > > Hi community, > > > in the context of HIVE-26196 > > > <https://issues.apache.org/jira/browse/HIVE-26196> we started > > considering > > > the adoption of SonarCloud <https://sonarcloud.io/features> analysis > for > > > Apache Hive to promote data-driven code quality improvements and to > allow > > > reviewers to focus on the conceptual part of the changes by helping > them > > > spot trivial code smells, security issues and bugs. > > > > > > SonarCloud has already been adopted and integrated into a few top > Apache > > > projects like DolphinScheduler <https://dolphinscheduler.apache.org/> > > and Apache > > > Jackrabbit FileVault <https://jackrabbit.apache.org/filevault/>. > > > > > > For those who don't know, Sonar is a code analysis tool, the initial > > > adoption would aim at tracking code quality for the master branch, and > > > making the PRs' review process easier, by allowing to compare which > > > code/security issues a PR solved/introduced with respect to the main > > branch. > > > > > > We already have a Hive-dedicated project under the Apache foundation's > > > SonarCloud account: > > https://sonarcloud.io/project/overview?id=apache_hive. > > > > > > In what follows I will highlight the main points of interest: > > > > > > 1) sonar adoption scope: > > > For the time being a descriptive approach (just show the analysis and > > > associated metrics) could be adopted, delaying a prescriptive one > (i.e., > > > quality gates based on the metrics for PRs' mergeability) to a later > time > > > where we have tested SonarCloud for long enough to judge that it could > > be a > > > sensible move. > > > > > > 2) false positives: > > > Sonar suffers from false positives, but they can be marked as such from > > > the web UI: (source https://docs.sonarqube.org/latest/faq/#header-1) > > > > > > How do I get rid of issues that are False-Positives? > > >> False-Positive and Won't Fix > > >> You can mark individual issues False Positive or Won't Fix through the > > >> issues interface. If you're using PR analysis provided by the > Developer > > >> Edition, issues marked False Positive or Won't Fix will retain that > > status > > >> after merge. This is the preferred approach. > > > > > > > > >> //NOSONAR > > >> For most languages, SonarQube supports the use of the generic > mechanism: > > >> //NOSONAR at the end of the line of the issue. This will suppress all > > >> issues - now and in the future - that might be raised on the line. > > > > > > > > > For the time being, I think that marking false positives via the UI is > > > more convenient than using "//NOSONAR", but this can be discussed > > further. > > > > > > 3) test code coverage: > > > > > > Due to the specific structure of the ptest infra (split execution and > > > other peculiarities), we are not yet supporting test code coverage, > this > > > can be added at a later stage, in the meantime all the code quality and > > > security metrics are available. > > > > > > 4) what will be analyzed: > > > > > > the master branch and each open PR > > > > > > 5) integration with github: > > > > > > SonarCloud integrates with GitHub in two ways, the first one is an > > > additional item in the list of checks (where you have the spell > checking, > > > CI result etc.) that will just say Passed/Not Passed and provide a link > > for > > > all the details, the second is a "summary" comment under the PR > > > highlighting the main info (you can see an example here > > > <https://github.com/apache/hive/pull/3254#issuecomment-1206641629>). > > > > > > The second integration can be disabled if we consider that the first > one > > > is enough, and that if we want to dig more we can open the associated > > link > > > for the full analysis in SonarCloud. > > > > > > 6) analysis runtime: > > > > > > In CI the full analysis takes around 30 minutes, but this step is > > executed > > > in parallel with the test split tasks and won't add to the total > runtime. > > > For PRs SonarCloud detects unchanged files and avoids analysing them, > so > > > the runtime there is expected to be lower. > > > > > > I'd like to hear your thoughts on this, and I am looking for reviewers > > for > > > the PR https://github.com/apache/hive/pull/3339. > > > > > > Best regards, > > > Alessandro > > > > > >
