C#: Autobuilder fixes#609
Conversation
This commit fixes a few issues that were identified during the last dist upgrade, and which were introduced/revealed on 836daaf. - Expand environment variables that are passed from `lgtm.yml` to the autobuilder, for example `solution: $LGTM_SRC/mysolution.sln`. - Distinguish between when a build rule is applied automatically and when it is applied manually via `lgtm.yml`. - Catch `FileNotFoundException`s when parsing project files and solution files.
hvitved
added
C#
depends on internal PR
calumgrant
left a comment
calumgrant
left a comment
There was a problem hiding this comment.
A few small things. My main question is whether we should be supporting $ on Windows, or just use $ on all platforms.
| Select(s => AsStringWithExpandedEnvVars(s, actions)).ToArray(); | ||
| } | ||
|
|
||
| static readonly Regex linuxEnvRegEx = new Regex(@"\$([A-Z_][A-Z_0-9]*)", RegexOptions.Compiled); |
| Select(s => AsStringWithExpandedEnvVars(s, actions)).ToArray(); | ||
| } | ||
|
|
||
| static readonly Regex linuxEnvRegEx = new Regex(@"\$([A-Z_][A-Z_0-9]*)", RegexOptions.Compiled); |
There was a problem hiding this comment.
Perhaps also lowercase letters.
|
|
||
| // `Environment.ExpandEnvironmentVariables` only works with Windows-style | ||
| // environment variables | ||
| var windowsStyle = actions.IsWindows() |
There was a problem hiding this comment.
We should probably support both expansion styles on all platforms. In particular, the documentation doesn't even mention the Windows-style at all, and users may expect it to work using $ even on Windows.
There was a problem hiding this comment.
LGTM_SRC is introduced as an environment variable here, so I think we should use the syntax for environment variables on the given platform. In the same way, if LGTM_SRC is used in a build_command, it is interpreted as a command script on Windows, so will have to use %LGTM_SRC% instead of $LGTM_SRC.
There was a problem hiding this comment.
That's a good argument.
hvitved
force-pushed
the
csharp/autobuilder/fixes
branch
from
7e999f7 to
1e9fe00
Compare
Go: Add Log Injection query (CWE-117)
3 participants
This commit fixes a few issues that were identified during the last dist upgrade, and which were introduced/revealed on 836daaf.
lgtm.ymlto the autobuilder, for examplesolution: $LGTM_SRC/mysolution.sln.lgtm.yml.FileNotFoundExceptions when parsing project files and solution files.