softgitron commented on a change in pull request #419:
URL: https://github.com/apache/incubator-ratis/pull/419#discussion_r576213992
##########
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:
Sorry, my apologies. I misunderstood and miss-evaluated some points.
> 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.
The worked previously as I described, but you actually made condition more
usable in your patch set. Instead of just preventing environment variables to
be set, condition now actually prevents whole step from running. Condition
makes perfect sense now.
> 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)
You may notice, that I'm not especially experienced with the open source
development. I always thought that development is done mainly using branches,
but I just noticed that even this pull request refers to a fork. This clause
refers to a hard forks where project totally divides into two different
projects.
There are no significant advantages for creating separate Sonar projects for
forks. Only on some special occasions this may come in handy. For example Sonar
Cloud shows full coverity statistics only on the master branch. It was crucial
for #423 to be able to push directly to the master to verify that everything is
working as intended. Another slight advantage from having separate Sonar would
be that developer could use full Sonar analysis, before even making a pull
request. However, I think, this is not especially usefull thing to do.
> Just to remind, after Ratis graduates as TLP sonar.sh should be also
updated with new -Dsonar.organization and -Dsonar.projectKey
Actually there is no need to update project organization after TLP status,
but I expect that projectKey should be updated by removing word incubator from
the key.
All in all, I think this pull request is good and can be merged as is. Maybe
these comments will be useful for some future request.
----------------------------------------------------------------
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]
