Enable VMR validation in repo PRs #15840
Conversation
MichaelSimons
left a comment
MichaelSimons
left a comment
There was a problem hiding this comment.
Feel free to address some of my comments in follow-ups.
There was a problem hiding this comment.
The name of this template doesn't resonate with me. "sync" instead of pull updates makes more sense to me. The steps all use the sync terminology.
There was a problem hiding this comment.
Easy to fix - I agree with proposal. I did, however, use the original template (and its name) from sdk repo, with some updated steps.
There was a problem hiding this comment.
Any preference: vmr-sync.yml or vmr-sync-updates.yml?
There was a problem hiding this comment.
Hmm, for this change I'd need to update the VMR first.
There was a problem hiding this comment.
VMR change needs to be merged first: dotnet/dotnet#594
| clean: true | ||
|
|
||
| - checkout: self | ||
| displayName: Clone dotnet/<repo> |
There was a problem hiding this comment.
Is it possible to inject the actual repo name here?
There was a problem hiding this comment.
We might be able to - let me check.
| displayName: Label PR commit | ||
| workingDirectory: $(Agent.BuildDirectory)/repo | ||
|
|
||
| - script: | |
There was a problem hiding this comment.
Thoughts on using template conditions to avoid the platform steps from appearing? It would simplify the steps displayed.
There was a problem hiding this comment.
Yeah, that's better. I have followed the original implementation of several steps (the sync ones).
I'd still want to keep the unique names of those steps, i.e. keep the platform, Unix, Windows.
There was a problem hiding this comment.
Hmm, this doesn't seem to work right. It always uses Unix steps - Agent.Os variable is supposedly evaluated early and reused in all templates. I need to revert this change.
There was a problem hiding this comment.
I've reverted this change to use runtime step conditions instead of early template conditions - e96f18c
| - script: | | ||
| vmr_sha=$(grep -oP '(?<=Sha=")[^"]*' $(Agent.BuildDirectory)/repo/eng/Version.Details.xml) | ||
| echo "##vso[task.setvariable variable=vmr_sha]$vmr_sha" | ||
| displayName: Obtain the vmr sha from Version.Details.xml (Linux) |
There was a problem hiding this comment.
This step used "(Linux)" while others use "(Unix)". They should be consistent.
There was a problem hiding this comment.
It might actually be better to use (Unix) which is the name used in old (existing) steps. I used (Linux) for new steps, but that is not completely correct, as those steps would be used on MacOS as well.
| endpoint: dotnet | ||
|
|
||
| stages: | ||
| - template: /eng/pipelines/templates/stages/vmr-build.yml@vmr |
There was a problem hiding this comment.
I think this template should be exposing some options to control what is run. Not all repos should need to run all of the lite configuration. Specifically SBRP and SBE do not need to run anything beside the SB legs. Not everyone may want to run stage 2 SB. I see value in some repos just wanting to run a single quick leg.
There was a problem hiding this comment.
This would need to be done in a follow up. There were plenty of recent changes in VMR build templates that have removed some of the properties used to condition build types, i.e. removal of isSourceOnlyBuild.
|
Created dotnet/dotnet#1057 to track the remaining work. |
|
Hmm, checks are failing due to the following, likely in @mmitche before I make changes to add telemetry logging, is this the right way, or should I add the new scripts to the exclusion list, i.e. arcade/eng/configure-toolset.ps1 Line 3 in 9bbce22 |
|
I've excluded the vmr-sync scripts from telemetry verification, with 0894fa4 |
2 participants
Contributes to: dotnet/arcade-services#4539
This PR brings necessary pipeline templates and associated scripts, for updating and building VMR as part of repo PR verification.
vmr-syncscripts use theDarctool, which required a tools manifest file in.config. This will need to be added to any repo that would want to onboard this experience. I could not find a different way to enable this, and it just follows what we had insdkbefore.vmr-syncscripts is the same as in https://github.com/dotnet/sdk/pull/48351/files#diff-05db8cf030b53040062f45ed99f7c8b96efdc35c5e90bc52e03db5fd14c472e8NuGetforsdkrepo. That will be implemented in a follow up PR insdkrepo, so that it can be properly tested first. There is some new code, invmr-syncfor doing the additional syncs, but I believe it's untested or would require further tweaks.