Re-enable sdk-diff approval stage

dotnet / source-build

A repository to track efforts to produce a source tarball of the .NET Core SDK and all its components

MIT License

264 stars 129 forks source link

Re-enable sdk-diff approval stage #4461

Closed NikolaMilosavljevic closed 1 month ago

NikolaMilosavljevic commented 2 months ago

Sdk-diff approval stage was disabled with https://dev.azure.com/dnceng/internal/_git/dotnet-release/pullrequest/40256

Mirror and Release stages where being skipped, when sdk-diff run was successful.

Sample build where stages were blocked (skipped): https://dev.azure.com/dnceng/internal/_build/results?buildId=2470263&view=results

ellahathaway commented 2 months ago

I have some changes to fix the skipping issue. They haven't been tested yet due to the release taking priority, but I'll update my branch with these changes once https://dev.azure.com/dnceng/internal/_git/dotnet-release/pullrequest/40256 gets merged.

ellahathaway commented 1 month ago

This is a bit tricky. Azdo templates do not currently allow for the conditioning of approval gates, meaning that the gate either gets skipped (and subsequent gates that depend on that approval gate also get skipped) or we force approval regardless of the outcome of the SDK diff tests.

I see a few paths forward here: 1) Remove the sdk-diff approval stage entirely and go back to the manual checking of the tests 2) Always force approval for the sdk-diff tests in official runs and test runs (non-validation) regardless of the test outcome 3) Always force approval for the sdk-diff tests in test runs (non-validation) regardless of the test outcome and warn of failures in official runs

I like the 3rd option the best, but I'd like to hear others' thoughts.

cc @dotnet/source-build-internal

mthalman commented 1 month ago

@ellahathaway - For the 3rd option, can you describe what the UX would be for the 3 pipeline types (validation, test, official), both for a successful and failed diff result?

ellahathaway commented 1 month ago

@ellahathaway - For the 3rd option, can you describe what the UX would be for the 3 pipeline types (validation, test, official), both for a successful and failed diff result?

Sure thing.

Validation: Warning on sdk diff failures, sdk diff approval gate does not exist -> mirror stage automatically runs after pre-release stage
Official: Warning on sdk diff failures, sdk diff approval gate does not exist -> mirror stage automatically runs after pre-release stage
Test: Warning on sdk diff failures, sdk diff approval gate is required regardless of sdk diff success or failures -> mirror stage will not run until sdk diff approval gate is approved.

For context, I can condition the existence of the approval gate with ${{ if eq(parameters.resourceType, '<type>' }}, but I cannot condition its existence with ${{ if eq(dependencies.PreRelease.outputs['PreRelease.VerifySdkDiffRun.RunSucceeded'], 'false') }}.

mthalman commented 1 month ago

I lean towards keeping the Official and Test builds as similar as possible. Is there a compelling reason to skip the approval for Official?

ellahathaway commented 1 month ago

I lean towards keeping the Official and Test builds as similar as possible. Is there a compelling reason to skip the approval for Official?

That makes sense. I don't have a very compelling reason to skip the approval stage for official runs other than that it'd be a repeat from the test run. I'm okay with requiring the approval in test and official runs and skipping the approval in validation runs.

ellahathaway commented 1 month ago

PR with fix (internal Microsoft link)