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

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

matthid and others added 2 commits Jul 16, 2019
Release 5.15.4
@Lutando

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

build failed 😢

@matthid

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
@matthid

matthid Jul 22, 2019

Collaborator

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

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

@Lutando

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Collaborator

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 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 fsharp:release/next Aug 25, 2019
3 checks passed
3 checks passed
FAKE-CI #1176 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matthid

This comment has been minimized.

Copy link
Collaborator

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 referenced this pull request Aug 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.