[pipeline generator] Build out weekly test pipelines for all services and fix scheduling #2484
Conversation
benbp
added
the
Central-EngSys
benbp
force-pushed
the
benbp/pipeline-gen-update-existing
branch
from
63be900 to
86df19c
Compare
benbp
changed the title
|
The following pipelines have been queued for testing: |
weshaggard
left a comment
weshaggard
left a comment
There was a problem hiding this comment.
Couple comments/suggestions but otherwise looks good.
.../Azure.Sdk.Tools.PipelineGenerator/Conventions/WeeklyIntegrationTestingPipelineConvention.cs
Show resolved
Hide resolved
|
The following pipelines have been queued for testing: |
…eline name collisions
|
The following pipelines have been queued for testing: |
|
The following pipelines have been queued for testing: |
tools/pipeline-generator/Azure.Sdk.Tools.PipelineGenerator/Conventions/PipelineConvention.cs
Show resolved
Hide resolved
|
|
||
| logger.LogInformation("Found {0} components", components.Count()); | ||
|
|
||
| if (HasPipelineDefinitionNameDuplicates(pipelineConvention, components)) |
There was a problem hiding this comment.
This might cause our pipeline generation to start failing so you need to be careful when you enable it for the first time.
There was a problem hiding this comment.
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.
|
|
||
| foreach (var component in components) | ||
| { | ||
| var definitionName = convention.GetDefinitionName(component); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Somewhere in CreateOrUpdateDefinitionAsync?
There was a problem hiding this comment.
GetDefinitionName is just a string building function, for example:
It's less efficient to do a separate check here as opposed to adding this logic incrementally in the scan step, but I think it's simpler this way and won't have any noticeable impact.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
The following pipelines have been queued for testing: |
|
The following pipelines have been queued for testing: |
|
Hello @azure-sdk! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
/check-enforcer override |
3 participants
This PR updates the weekly test pipeline to run by default for all services (which will behave like our weekday live tests). It also fixes a scheduling/bucketing problem where the weekly tests were running on both Saturday and Sunday.
Update: this PR also adds name collision detection for pipeline definitions, with a helpful log message for a workaround. Resolves #2474
Resolves #2377