-
Notifications
You must be signed in to change notification settings - Fork 229
[pipeline generator] Build out weekly test pipelines for all services and fix scheduling #2484
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
Changes from all commits
419e180
41f6fc1
86df19c
180cdde
a50c129
5fb7075
c5abe56
c445f30
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -61,6 +61,7 @@ private static CommandLineApplication PrepareApplication(CancellationTokenSource | |||
| var destroyOption = app.Option("--destroy", "Use this switch to delete the pipelines instead (DANGER!)", CommandOptionType.NoValue); | ||||
| var debugOption = app.Option("--debug", "Turn on debug level logging.", CommandOptionType.NoValue); | ||||
| var noScheduleOption = app.Option("--no-schedule", "Don't create any scheduled triggers.", CommandOptionType.NoValue); | ||||
| var overwriteTriggersOption = app.Option("--overwrite-triggers", "Overwrite existing pipeline triggers (triggers may be manually modified, use with caution).", CommandOptionType.NoValue); | ||||
|
|
||||
| app.OnExecute(() => | ||||
| { | ||||
|
|
@@ -83,6 +84,7 @@ private static CommandLineApplication PrepareApplication(CancellationTokenSource | |||
| openOption.HasValue(), | ||||
| destroyOption.HasValue(), | ||||
| noScheduleOption.HasValue(), | ||||
| overwriteTriggersOption.HasValue(), | ||||
| cancellationTokenSource.Token | ||||
| ).Result; | ||||
|
|
||||
|
|
@@ -146,6 +148,7 @@ public async Task<ExitCondition> RunAsync( | |||
| bool open, | ||||
| bool destroy, | ||||
| bool noSchedule, | ||||
| bool overwriteTriggers, | ||||
| CancellationToken cancellationToken) | ||||
| { | ||||
| try | ||||
|
|
@@ -167,7 +170,8 @@ public async Task<ExitCondition> RunAsync( | |||
| devOpsPathValue, | ||||
| prefix, | ||||
| whatIf, | ||||
| noSchedule | ||||
| noSchedule, | ||||
| overwriteTriggers | ||||
| ); | ||||
|
|
||||
| var pipelineConvention = GetPipelineConvention(convention, context); | ||||
|
|
@@ -180,6 +184,12 @@ public async Task<ExitCondition> RunAsync( | |||
| } | ||||
|
|
||||
| logger.LogInformation("Found {0} components", components.Count()); | ||||
|
|
||||
| if (HasPipelineDefinitionNameDuplicates(pipelineConvention, components)) | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might cause our pipeline generation to start failing so you need to be careful when you enable it for the first time.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tested locally, it passes for all repos (up, ci, tests, weekly conventions). Minor issue with local copies of node_modules causing duplicates, but that's an edge case that I think is ok and won't affect the pipeline. |
||||
| { | ||||
| return ExitCondition.DuplicateComponentsFound; | ||||
| } | ||||
|
|
||||
| foreach (var component in components) | ||||
| { | ||||
| logger.LogInformation("Processing component '{0}' in '{1}'.", component.Name, component.Path); | ||||
|
|
@@ -236,5 +246,36 @@ private IEnumerable<SdkComponent> ScanForComponents(string path, string searchPa | |||
| var components = scanner.Scan(scanDirectory, searchPattern); | ||||
| return components; | ||||
| } | ||||
|
|
||||
| private bool HasPipelineDefinitionNameDuplicates(PipelineConvention convention, IEnumerable<SdkComponent> components) | ||||
| { | ||||
| var pipelineNames = new Dictionary<string, SdkComponent>(); | ||||
| var duplicates = new HashSet<SdkComponent>(); | ||||
|
|
||||
| foreach (var component in components) | ||||
| { | ||||
| var definitionName = convention.GetDefinitionName(component); | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIRC this causes a network call each time and we already do this in other places. Should we just add the dictionary building and checking there instead?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Somewhere in CreateOrUpdateDefinitionAsync?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Line 19 in baefa12
It's less efficient to do a separate check here as opposed to adding this logic incrementally in the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK as long as we are just processing things in memory and not making more network calls I'm cool with making it an explicit check.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also explicitly did it here vs. per update call, because I wanted to be able to collect all collisions in case there are more than 2, so that we can fail with full information about what needs to be fixed. |
||||
| if (pipelineNames.TryGetValue(definitionName, out var duplicate)) | ||||
| { | ||||
| duplicates.Add(duplicate); | ||||
| duplicates.Add(component); | ||||
| } | ||||
| else | ||||
| { | ||||
| pipelineNames.Add(definitionName, component); | ||||
| } | ||||
| } | ||||
|
|
||||
| if (duplicates.Count > 0) { | ||||
| logger.LogError("Found multiple pipeline definitions that will result in name collisions. This can happen when nested directory names are the same."); | ||||
| logger.LogError("Suggested fix: add a 'variant' to the yaml filename, e.g. 'sdk/keyvault/internal/ci.yml' => 'sdk/keyvault/internal/ci.keyvault.yml'"); | ||||
| var paths = duplicates.Select(d => $"'{d.RelativeYamlPath}'"); | ||||
| logger.LogError($"Pipeline definitions affected: {String.Join(", ", paths)}"); | ||||
|
|
||||
| return true; | ||||
| } | ||||
|
|
||||
| return false; | ||||
| } | ||||
| } | ||||
| } | ||||
Uh oh!
There was an error while loading. Please reload this page.