Add PR pipeline stage to validate VMR changes #1270
Conversation
63faec3 to
910d34f
Compare
|
@adamzip as we spoke earlier - it would probably be good to comment out the other stages so that we don't block compute for no reason (and since there are iterations to be expected) |
|
@jkoritzinsky please note that there will be a whole suite of checks we will run and many will use existing functionality from Maestro to parse For this reason, C# will be more tenable long-term. Existing GHA checks from Ella will be ported to C# too. |
e056e68 to
78a6e64
Compare
| @@ -1,22 +0,0 @@ | |||
| name: 'Check Protected Files for Changes' | |||
There was a problem hiding this comment.
This is probably not desired? Or is it one of the tests?
There was a problem hiding this comment.
This is the github workflow that checks the tooling files that the VMR change validation now checks instead
| Console.WriteLine(""); | ||
| Console.WriteLine(""); |
There was a problem hiding this comment.
| Console.WriteLine(""); | |
| Console.WriteLine(""); | |
| Console.WriteLine(); | |
| Console.WriteLine(); |
| // Run Submodule Validation | ||
| Console.WriteLine(""); | ||
| Console.WriteLine(""); | ||
| Console.WriteLine("2. Starting Submodule Validation..."); |
There was a problem hiding this comment.
Shouldn't this be rather logged from within the method itself?
| Console.WriteLine(""); | ||
| Console.WriteLine(""); | ||
| Console.WriteLine("3. Starting Exclusion File Validation..."); |
There was a problem hiding this comment.
The repetition of these "sections" is not great. Especially since when we will be defining more of these.
I'd imagine we define a list of validations and then have code to loop through them and run/output them.
The list could just be a list of types and the validations could inherit from an interface/base class so that we just run some Validate() method on each one. We can instantiate each from DI.
| { | ||
| foreach (var line in lines) | ||
| { | ||
| Console.WriteLine($"##vso[task.logissue type=error] {line}"); |
There was a problem hiding this comment.
Looking at this page (where devs usually go first): https://github.com/dotnet/dotnet/pull/1270/checks?check_run_id=46502889155
we should consider which lines should be outputted with ##error only to make it more readable
| JArray? mappings = null; | ||
| try | ||
| { | ||
| var jsonRoot = JObject.Parse(fileContents); | ||
|
|
||
| if (jsonRoot.TryGetValue("mappings", out JToken? mappingsToken) && mappingsToken?.Type == JTokenType.Array) | ||
| { | ||
| mappings = (JArray)mappingsToken; | ||
| } | ||
| else | ||
| { | ||
| Console.WriteLine($"Cannot find mappings in {SourceMappingsPath}"); | ||
| return []; | ||
| } | ||
| } |
There was a problem hiding this comment.
We could just parse the file using DarcLib into an existing POCOs with an existing schema to be more type safe. This whole method could be a 2 liner then.
|
|
||
|
|
||
|
|
||
| internal static string RunGitCommand(string arguments) |
There was a problem hiding this comment.
It might be better to re-use ProcessManager since we will need DarcLib anyway
| return output; | ||
| } | ||
|
|
||
| internal static void AddProcessingMessage(List<ProcessingMessage> processingMessages, ProcessingMessage processingMessage) |
There was a problem hiding this comment.
Are we buffering the messages just for the sake of counting them at the end or is there another reason?
I think it'd be just better to output them right away and if we need the count, increment some counter then?
|
|
||
| var newExclusionRules = GetExclusionPatternsFromBranch("HEAD"); | ||
|
|
||
| if (newExclusionRules == null || !newExclusionRules.Any()) |
There was a problem hiding this comment.
It's entirely possible we will not have any exclusions and that is a valid scenario
| private static string FindGitRepoRoot(string startDirectory) | ||
| { | ||
| var dir = new DirectoryInfo(startDirectory); | ||
| while (dir != null) | ||
| { | ||
| var gitPath = Path.Combine(dir.FullName, ".git"); | ||
| if (Directory.Exists(gitPath) || File.Exists(gitPath)) | ||
| { | ||
| return dir.FullName; | ||
| } | ||
| dir = dir.Parent; | ||
| } | ||
| throw new InvalidOperationException("Could not find the Git repository root."); | ||
| } |
There was a problem hiding this comment.
This method is probably also available in Darclib
test pr