softgitron commented on a change in pull request #419:
URL: https://github.com/apache/incubator-ratis/pull/419#discussion_r575103692
##########
File path: .github/workflows/post-commit.yml
##########
@@ -151,3 +146,28 @@ jobs:
- name: Delete temporary build artifacts
run: rm -rf ~/.m2/repository/org/apache/ratis
if: always()
+ sonar:
+ name: sonar
+ runs-on: ubuntu-18.04
+ if: github.repository == 'apache/incubator-ratis' && github.event_name !=
'pull_request'
Review comment:
I have actually also investigated this issue on my personal
[fork](https://github.com/softgitron/incubator-ratis/commit/7a6541f2237b434b3767dae409493be296cd3eea)
and ended up with fairly similar modifications. I think main functionality on
this commit is good and should be implemented as is. What comes to this
specific code line, I would drop check for repository for following reasons:
- This check does not add value. Sonar analysis will still fail couple of
seconds later in the sonar.sh file due to missing environment variables so it
is only matter of which error message you will get.
- This makes usage of the sonar checks annoying on the forks, because line
must be always patched away. (I'm not expecting that forking will be especially
common for an open source project, but still improvement)
Just to remind, after Ratis graduates as TLP sonar.sh should be also updated
with new `-Dsonar.organization` and `-Dsonar.projectKey` It would be nice if
those fields could update themselves, but that would possibly make the script
slightly too complex. So maybe it is better to leave those parameters as is.
Again those parameters are slight inconvenience for forgers, but I think it
would be wrong to move those also to Github secrets for example for easier
configurations since they are not really secrets.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
