Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Organization field for SonarQube tester #2358

Merged
merged 3 commits into from
Aug 25, 2019

Conversation

Lutando
Copy link
Contributor

@Lutando Lutando commented Jul 17, 2019

Description

This PR adds the ability to supply the organization argument to the sonarqube scanner. This is important when sending the sonarscanner results to sonarcloud.io

@Lutando
Copy link
Contributor Author

Lutando commented Jul 17, 2019

build failed 😢

@matthid
Copy link
Member

matthid commented Jul 22, 2019

I manually triggered it again and it went green: https://dev.azure.com/fakebuild/FSProjects/_build/results?buildId=1170
Error looks like you were a bit unlucky...

@@ -48,7 +55,7 @@ module Fake.Testing.SonarQube

let args =
match call with
| Begin -> "begin /k:\"" + parameters.Key + "\" /n:\"" + parameters.Name + "\" /v:\"" + parameters.Version + "\" " + setArgs + cfgArgs
| Begin -> "begin /k:\"" + parameters.Key + "\" /n:\"" + parameters.Name + "\" /v:\"" + parameters.Version + "\" " + setArgs + cfgArgs + orgArgs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we would use the Arguments-API instead of string concatenation. Other modules like DotNet.Cli already use it. It also eases writing tests without actually starting anything.
Any interest in pushing this into that direction? The docs are here: https://fake.build/core-process.html

@Lutando
Copy link
Contributor Author

Lutando commented Jul 22, 2019

I can do this :). Just went on vacation so let's say next week I can do it.

@Lutando
Copy link
Contributor Author

Lutando commented Aug 7, 2019

I tried to make it work with the arguments API but I had problems and ill try to explain by trying to explain out most command line interfaces work. Most CLIs have a pretty industry standard CLI that goes:

command -flag arg

Now with SonarQube it looks more like this

command /flag:arg

now it is possible to accomodate for this with the arguments API but there are some commands in the SonarQube CLI that fail to validate if you do not surround the argument in quotes. So theres a second requirement that we need to accomodate for and that is

command /flag:"arg"

The problem with the last one is that the Arguments api strips quotes from the strings that you provide it.

When I supply the right arguments to SonarQube CLI it bombs since the arguments have been stripped of these important quotes. I hope I am not being crazy here but I think I have tried everything and that seems to be how it works. And my assumptions are confirmed by how the current string appending implementation works on the current version of this package.

@matthid
Copy link
Member

matthid commented Aug 12, 2019

Nice analysis, yes there are unfortunately some windows tool, which do some strange argument parsing like this. In theory it shouldn't matter if you put quotes or don't (when no space is in the arguments). Therefore the argument parser 'simplifies' expressions.

There is a workaround which allows to provide a raw string unmodified. However, this basically makes the arguments api unusable for this module. So for testing we can only generate the raw string and compare that one and maybe use some primitives for escaping.

Sorry, I didn't expect sonar to be of this kind, this usually hits older windows software...

@Lutando
Copy link
Contributor Author

Lutando commented Aug 13, 2019

No problem, I still learned a lot so for me its not like it was time wasted. So how do you want to proceed? Should we use my previous commit that works but doesn't leave the codebase in a better condition or is there another way around this?

@matthid
Copy link
Member

matthid commented Aug 13, 2019

I probably will take a look eventually, because we need a good way to handle this as well. But it could take a while :/
Ideally we have a good workaround for this - even if it means adding some APIs to the Core. Additionally, we want a good story to add tests - even for these kind of weird CLI-tools.

@matthid matthid added the needs-tests This PR needs tests in order to be accepted label Aug 17, 2019
@matthid matthid self-assigned this Aug 17, 2019
@matthid matthid closed this in d349e43 Aug 25, 2019
@matthid matthid merged commit 3cd8ddc into fsprojects:release/next Aug 25, 2019
@matthid
Copy link
Member

matthid commented Aug 25, 2019

@Lutando FYI, solved it like this
Let's see if it works. In any case it will "break" your "workaround" of #2356

@matthid matthid mentioned this pull request Aug 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-tests This PR needs tests in order to be accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No ability to supply organization in SonarQube
2 participants