Add logging when environment variable parsing fails #1251
Conversation
andrei-epure-sonarsource
force-pushed
the
andrei/1245
branch
from
47867ec
to
8e5eb6d
Compare
|
@csaba-sagi-sonarsource please have a look at the draft - you need to have a look at it commit by commit (skip the first one) - after #1246 gets merged I will do a rebase Tests were green, I've rewritten the commit history to be easier to read. |
its/src/test/java/com/sonar/it/scanner/msbuild/ScannerMSBuildTest.java
Outdated
Show resolved
Hide resolved
Tests/SonarScanner.MSBuild.Common.Test/AnalysisConfig/AnalysisConfigExtensionsTests.cs
Show resolved
Hide resolved
| @@ -53,7 +53,7 @@ public void CreateSummaryData_ConfigIsNull_Throws() | |||
| { | |||
| // Arrange | |||
There was a problem hiding this comment.
I suggest creating a [TestInitialize] method and create the logger there and use everywhere in the class from a private field.
|
The URI code smell - possible refactoring rabbit hole and I don't currently understand the TFS area well enough - #1252 - if there's time in this sprint. Pragmatically, there's some tickets with higher user value than this (although this code smell can hide possible bugs). |
|
This code smell means removing |
andrei-epure-sonarsource
force-pushed
the
andrei/1245
branch
from
4c4c60a
to
2e7aa2d
Compare
| @@ -272,7 +272,7 @@ private static Path getNuGetPath(Orchestrator orch) { | |||
| return nugetPath; | |||
| } | |||
|
|
|||
| private static BuildResult runMSBuildQuietly(Orchestrator orch, Path projectDir, List<EnvironmentVariable> environmentVariables, String... arguments) { | |||
| public static BuildResult runMSBuildQuietly(Orchestrator orch, Path projectDir, List<EnvironmentVariable> environmentVariables, String... arguments) { | |||
There was a problem hiding this comment.
note: the layout of the whole file is a bit of a mess (private, public methods are all around the place and not grouped by visibility), but I don't think this IT code is the best use of our time in changing the structure (public , then private methods)
| { | ||
| logger?.LogError(Resources.ERROR_FailedParsePropertiesEnvVar, ENV_VAR_KEY); | ||
| logger.LogError(Resources.ERROR_FailedParsePropertiesEnvVar, ENV_VAR_KEY, ex.Message); |
There was a problem hiding this comment.
this will fail the build, which is ok in this case (otherwise, users will be confused)
andrei-epure-sonarsource
force-pushed
the
andrei/1245
branch
from
2e7aa2d
to
3eedf6d
Compare
| @@ -48,7 +46,7 @@ public static bool TryCreateProvider(ILogger logger, out IAnalysisPropertyProvid | |||
| provider = new EnvScannerPropertiesProvider(Environment.GetEnvironmentVariable(ENV_VAR_KEY)); | |||
| return true; | |||
| } | |||
| catch (Exception ex) when (ex is JsonException || ex is IOException) | |||
There was a problem hiding this comment.
JsonException is in Newtonsoft 12 :|
hence failures on .NET Core 2.1
🤦
There was a problem hiding this comment.
Would it make sense to leave a comment here, so it will be fixed once NET 2.1 version is removed?
There was a problem hiding this comment.
I'll do a separate PR to avoid postponing merging this one.
I think there's multiple things:
- filtering down the exception
- general cleanup after removing .NET 2.1
There was a problem hiding this comment.
|
Kudos, SonarCloud Quality Gate passed! |
Fix #1245