Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@AndriySvyryd PTAL |
There was a problem hiding this comment.
Pull request overview
Enables Arcade’s Helix Job Monitor in EF Core’s public and internal Azure DevOps pipelines so Helix submission jobs can stop after queueing, while a dedicated monitor job publishes test results and drives the final Helix status.
Changes:
- Add
Microsoft.DotNet.Helix.JobMonitordependency/version plumbing and pin the tool via.config/dotnet-tools.json. - Enable Helix Job Monitor behavior in
eng/helix.projwhenSYSTEM_ACCESSTOKENis available. - Add the
helix-job-monitor.ymljob template to both public and internal pipelines.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| eng/Version.Details.xml | Adds Helix Job Monitor dependency tracking entry. |
| eng/Version.Details.props | Introduces Helix Job Monitor version properties alongside other dotnet-dotnet dependencies. |
| eng/helix.proj | Turns on Helix Job Monitor mode when SYSTEM_ACCESSTOKEN is set. |
| azure-pipelines-public.yml | Adds Helix Job Monitor job template to the public pipeline (currently with an indentation issue). |
| azure-pipelines-internal-tests.yml | Adds Helix Job Monitor job template to the internal test pipeline and passes helixAccessToken. |
| .config/dotnet-tools.json | Pins the dotnet-helix-job-monitor tool version for dotnet tool restore. |
| - template: /eng/common/core-templates/job/helix-job-monitor.yml@self | ||
| parameters: | ||
| helixAccessToken: $(HelixApiAccessToken) | ||
| - stage: validate |
There was a problem hiding this comment.
You need to change the validate logic to also check whether the corresponding Helix monitor job succeeded
There was a problem hiding this comment.
Ideally, we should check the result of each individual Helix job to preserve the validation logic, though I am not sure whether this is currently possible to do here (feature request?)
Otherwise, add $helixJobMonitorResult to each item in $groupResults instead of failing outright
There was a problem hiding this comment.
Ideally, we should check the result of each individual Helix job to preserve the validation logic
What would be the benefit of this? The helix jobs no longer depend on the test results, they just send the tests off and then report green. If one fails, the monitor will fail too
There was a problem hiding this comment.
What would be the benefit of this? The helix jobs no longer depend on the test results, they just send the tests off and then report green. If one fails, the monitor will fail too
Right, the benefit would be from checking the helix monitoring jobs for test failures specific to that leg
There was a problem hiding this comment.
the benefit would be from checking the helix monitoring jobs for test failures specific to that leg
But the individual legs don't fail when there are test failures - as soon as the tests are sent to helix, they complete w/ success. Only the helix monitor job will ever fail for test failures.
There was a problem hiding this comment.
Only the helix monitor job will ever fail for test failures.
Yes and we need to make that failure more granular, so that we can continue to check only the relevant failures in the validation groups.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Preserve the Helix job monitor validation while retaining the retry aggregation improvements from main. Update the new JobMonitor dependency to the current dotnet build versions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Monitor job dependencies and no-Helix-job handling must be corrected in both pipelines.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Lite
| HelixAccessToken: $(_HelixAccessToken) | ||
| SYSTEM_ACCESSTOKEN: $(System.AccessToken) | ||
| DotNetBuildsInternalReadSasToken: $(dotnetbuilds-internal-container-read-token) | ||
| - template: /eng/common/core-templates/job/helix-job-monitor.yml@self |
| env: | ||
| HelixAccessToken: $(_HelixAccessToken) | ||
| SYSTEM_ACCESSTOKEN: $(System.AccessToken) | ||
| - template: /eng/common/core-templates/job/helix-job-monitor.yml |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Both pipeline monitor jobs can run before Helix submissions; add explicit dependencies and use always().
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
azure-pipelines-internal-tests.yml:667
- This invocation leaves the monitor's
dependsOnat its[]default, so Azure DevOps can startHelixJobMonitorin parallel with theHelix_*submission jobs; list order does not create a dependency. It can therefore observe no queued work and finish before the submitters release their agents. Pass explicit dependencies on the Helix jobs and usealways()so skipped jobs in filtered runs do not suppress the monitor.
azure-pipelines-public.yml:531 - This invocation leaves the monitor's
dependsOnat its[]default, so Azure DevOps can startHelixJobMonitorin parallel with theHelix_*submission jobs; list order does not create a dependency. It can therefore observe no queued work and finish before the submitters release their agents. Pass explicit dependencies on the Helix jobs and usealways()so skipped jobs in filtered runs do not suppress the monitor.
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
Enable Arcade's Helix Job Monitor for the public and internal test pipelines. Helix submission jobs can now release their agents after queueing work, while the monitor publishes test results and owns the final Helix status.