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

SCAN4NET-234 Use SONAR_SCANNER_OPTS to map the new properties #2314

Open
wants to merge 5 commits into
base: feature/MMF_4168
Choose a base branch
from

Conversation

sebastien-marichal
Copy link
Contributor

@sebastien-marichal sebastien-marichal commented Feb 4, 2025

@hashicorp-vault-sonar-prod hashicorp-vault-sonar-prod bot changed the title Use SONAR_SCANNER_OPTS to map the new properties SCAN4NET-234 Use SONAR_SCANNER_OPTS to map the new properties Feb 4, 2025
@sebastien-marichal sebastien-marichal marked this pull request as ready for review February 4, 2025 16:01
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about spaces in file path are handled correctly.

/// List of analysis settings supplied locally (on the
/// command line) that has to be passed to the scanner through
/// the SONAR_SCANNER_OPTS environment variable
/// </summary>

Choose a reason for hiding this comment

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

Suggested change
/// </summary>
/// <see href="https://github.com/SonarSource/sonar-scanner-cli/blob/7d791c2465384b71465a6c05d23174fefdbfa213/src/main/assembly/bin/sonar-scanner.bat#L72C66-L72C84">sonar-scanner.bat</see>
/// </summary>

result.Should().BeTrue();
}

CheckEnvVarExists("SONAR_SCANNER_OPTS", "-Dsome.property=value -Dsome.other.property=\"another value with #%\\/?*\"", mockRunner);

Choose a reason for hiding this comment

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

For file names, we can expect spaces in the path. Does AsSonarScannerArg escape these? Background: Any double quotes coming via command line to the dotnet scanner are stripped off (as far as I can tell), so we would pass the file path without quotes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AsSonarScannerArg does not escape the spaces :(

Can we safely force the double quote?
I remember we had trouble with quote escaping in the past when invoking the bat file.

Copy link

sonarqubecloud bot commented Feb 7, 2025

@@ -43,13 +43,15 @@ public static AnalysisConfig GenerateFile(ProcessedArgs localSettings,
IDictionary<string, string> serverProperties,
List<AnalyzerSettings> analyzersSettings,
string sonarQubeVersion,
string resolvedJavaExePath)
string resolvedJavaExePath,
ILogger logger)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding the ILogger raised S107, I accepted the issue.
I am open to suggestion to fix it.

I need the logger to create the OperatingSystemProvider for the mapping provider.
I am open to suggestions to fix it, I would like to avoid a full rework.

I could not use OperatingSystemProvider to detect the current OS however I would probably lose testability and would create some kind of duplications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants