Skip to content

Conversation

@cheenamalhotra
Copy link
Member

@cheenamalhotra cheenamalhotra commented Nov 13, 2025

Description

Adds a manual, parameter‑gated release stage to the signing pipeline enabling:

  • Internal or public NuGet publish (publishDestination)
  • Dry run preview (dryRun)
  • Optional symbol publishing (MDS only)
  • Human approval checklist (destination, preview, dry run, symbols, version, product)

Next Steps:

  • Run Dryrun and Approval workflow to validate changes ensuring configuration is up-to-date.
  • Make an attempt to publish MDS packages to internal feed.
  • Validate workflows for all 3 packages: MDS, MSS, AKV

Investigate:

  • Can symbols be published using the same "compound-publish-symbols-step" for AKV and MSS packages?

@cheenamalhotra cheenamalhotra requested a review from a team as a code owner November 13, 2025 00:29
Copilot AI review requested due to automatic review settings November 13, 2025 00:29
Copilot finished reviewing on behalf of cheenamalhotra November 13, 2025 00:32
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a manual release stage to the OneBranch signing pipeline, enabling controlled NuGet package publishing with approval gates. The implementation supports both internal and public publishing destinations, includes dry-run capability for testing, and integrates symbol publishing for the MDS product.

Key Changes:

  • Adds manual release parameters (destination, dry run, product) to the signing pipeline
  • Implements approval workflow with human validation before package publication
  • Creates templated release infrastructure supporting multiple products (MDS, MSS, AKV)

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
eng/pipelines/dotnet-sqlclient-signing-pipeline.yml Adds release parameters and invokes new release-stage template
eng/pipelines/common/templates/stages/release-stage.yml Defines manual release stage with approval and publish jobs
eng/pipelines/common/templates/jobs/approval-job.yml Implements manual validation job with release checklist
eng/pipelines/common/templates/jobs/publish-packages-job.yml Orchestrates package download and conditional publishing to internal/public feeds
eng/pipelines/common/templates/steps/publish-internal-feed-step.yml Handles internal feed publishing with dry-run support
eng/pipelines/common/templates/steps/publish-public-nuget-step.yml Handles NuGet.org publishing with dry-run support
eng/pipelines/common/templates/steps/list-packages-step.yml Lists packages for verification before publishing
eng/pipelines/common/templates/steps/publish-symbols-step.yml Updates symbol publishing to use boolean type and add AKV product support

Copilot AI review requested due to automatic review settings November 13, 2025 05:01
Copilot finished reviewing on behalf of cheenamalhotra November 13, 2025 05:04
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.

echo "Directory does not exist yet: $dir"
fi
fi
for f in $(find "${{ parameters.packagesGlob }}" -name "*.nupkg"); do
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The find command is incorrectly used here. The first argument should be a directory path, not a glob pattern. This will fail when the script tries to execute. Consider using: find "$(dirname '${{ parameters.packagesGlob }}')" -type f -name "$(basename '${{ parameters.packagesGlob }}')" or a simpler approach with shell globbing like for f in ${{ parameters.packagesGlob }}; do.

Suggested change
for f in $(find "${{ parameters.packagesGlob }}" -name "*.nupkg"); do
for f in ${{ parameters.packagesGlob }}; do

Copilot uses AI. Check for mistakes.
Comment on lines +42 to +45
for f in $(find "${{ parameters.packagesGlob }}" -name "*.nupkg"); do
echo "Push $f"
dotnet nuget push --source "$SRC" --api-key az "$f"
done
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dry run logic only prints files but doesn't skip the actual push operation. When dryRun is true, the script should exit after listing files, or the push loop (lines 40-43) should be conditionally executed only when dryRun is false. Currently, packages will be pushed even in dry run mode.

Suggested change
for f in $(find "${{ parameters.packagesGlob }}" -name "*.nupkg"); do
echo "Push $f"
dotnet nuget push --source "$SRC" --api-key az "$f"
done
if [ "${{ parameters.dryRun }}" != "true" ]; then
for f in $(find "${{ parameters.packagesGlob }}" -name "*.nupkg"); do
echo "Push $f"
dotnet nuget push --source "$SRC" --api-key az "$f"
done
fi

Copilot uses AI. Check for mistakes.
Comment on lines +53 to +59
packagesGlob: $(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg
- ${{ if eq(parameters.publishDestination, 'Public') }}:
- template: ../steps/publish-public-nuget-step.yml
parameters:
dryRun: ${{ parameters.dryRun }}
publicNuGetSource: ${{ parameters.publicNuGetSource }}
packagesGlob: $(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hardcoded artifact path $(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg contains 'MDS' in the path but is used for all product types (MDS, MSS, AKV). This will fail when publishing MSS or AKV packages. The path should either be parameterized based on the product type or use the downloaded artifact location from line 44: $(Pipeline.Workspace)/release/packages/*.nupkg.

Suggested change
packagesGlob: $(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg
- ${{ if eq(parameters.publishDestination, 'Public') }}:
- template: ../steps/publish-public-nuget-step.yml
parameters:
dryRun: ${{ parameters.dryRun }}
publicNuGetSource: ${{ parameters.publicNuGetSource }}
packagesGlob: $(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg
packagesGlob: $(Pipeline.Workspace)/release/packages/*.nupkg
- ${{ if eq(parameters.publishDestination, 'Public') }}:
- template: ../steps/publish-public-nuget-step.yml
parameters:
dryRun: ${{ parameters.dryRun }}
publicNuGetSource: ${{ parameters.publicNuGetSource }}
packagesGlob: $(Pipeline.Workspace)/release/packages/*.nupkg

Copilot uses AI. Check for mistakes.
Comment on lines +53 to +59
packagesGlob: $(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg
- ${{ if eq(parameters.publishDestination, 'Public') }}:
- template: ../steps/publish-public-nuget-step.yml
parameters:
dryRun: ${{ parameters.dryRun }}
publicNuGetSource: ${{ parameters.publicNuGetSource }}
packagesGlob: $(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hardcoded artifact path $(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg contains 'MDS' in the path but is used for all product types (MDS, MSS, AKV). This will fail when publishing MSS or AKV packages. The path should either be parameterized based on the product type or use the downloaded artifact location from line 44: $(Pipeline.Workspace)/release/packages/*.nupkg.

Suggested change
packagesGlob: $(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg
- ${{ if eq(parameters.publishDestination, 'Public') }}:
- template: ../steps/publish-public-nuget-step.yml
parameters:
dryRun: ${{ parameters.dryRun }}
publicNuGetSource: ${{ parameters.publicNuGetSource }}
packagesGlob: $(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg
packagesGlob: $(Pipeline.Workspace)/release/packages/*.nupkg
- ${{ if eq(parameters.publishDestination, 'Public') }}:
- template: ../steps/publish-public-nuget-step.yml
parameters:
dryRun: ${{ parameters.dryRun }}
publicNuGetSource: ${{ parameters.publicNuGetSource }}
packagesGlob: $(Pipeline.Workspace)/release/packages/*.nupkg

Copilot uses AI. Check for mistakes.
Comment on lines +19 to +20
default: '$(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg'

Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default value for packagesGlob is hardcoded to an MDS-specific path $(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg. This should not have a default value with 'MDS' hardcoded, as this template is intended to be reusable for all product types (MDS, MSS, AKV). Consider removing the default or making it generic.

Suggested change
default: '$(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg'

Copilot uses AI. Check for mistakes.

- name: packagesGlob
type: string
default: '$(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg'
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default value for packagesGlob is hardcoded to an MDS-specific path $(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg. This should not have a default value with 'MDS' hardcoded, as this template is intended to be reusable for all product types (MDS, MSS, AKV). Consider removing the default or making it generic.

Suggested change
default: '$(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg'

Copilot uses AI. Check for mistakes.
#################################################################################
parameters:
- name: dryRun
type: boolean
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Missing default value for required parameter. While dryRun parameter has a type of boolean, it lacks a default value. For consistency with other templates in this PR and to prevent errors when the parameter is not explicitly provided, consider adding default: false.

Suggested change
type: boolean
type: boolean
default: false

Copilot uses AI. Check for mistakes.
exit 1
fi
if [ "${{ parameters.dryRun }}" = "true" ]; then
echo "[DRY RUN] Listing packages targeted for push to: ${{ parameters.publicNuGetSource }}"
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message references ${{ parameters.publicNuGetSource }} but this parameter doesn't exist in this template. It should reference ${{ parameters.internalFeedSource }} instead to correctly show which feed is being targeted.

Suggested change
echo "[DRY RUN] Listing packages targeted for push to: ${{ parameters.publicNuGetSource }}"
echo "[DRY RUN] Listing packages targeted for push to: ${{ parameters.internalFeedSource }}"

Copilot uses AI. Check for mistakes.
#################################################################################
parameters:
- name: dryRun
type: boolean
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Missing default value for required parameter. While dryRun parameter has a type of boolean, it lacks a default value. For consistency with other templates in this PR and to prevent errors when the parameter is not explicitly provided, consider adding default: false.

Suggested change
type: boolean
type: boolean
default: false

Copilot uses AI. Check for mistakes.
dryRun: ${{ parameters.dryRun }}
publicNuGetSource: ${{ parameters.publicNuGetSource }}
packagesGlob: $(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg
- ${{ if and(parameters.publishSymbols, ne(parameters.dryRun, true)) }}:
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The condition ne(parameters.dryRun, true) should use eq(parameters.dryRun, false) for consistency with other conditionals in the codebase, or the double-negative logic could be simplified. Additionally, this condition prevents symbol publishing during dry runs, but the individual steps within publish-symbols-step.yml already have their own conditions. Consider whether this outer condition is necessary or if it should be documented why symbols aren't published during dry runs.

Suggested change
- ${{ if and(parameters.publishSymbols, ne(parameters.dryRun, true)) }}:
# Only publish symbols if requested and not a dry run
- ${{ if and(parameters.publishSymbols, eq(parameters.dryRun, false)) }}:

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We seem to have two "jobs" directories, and it's not clear which jobs belong where. Just food for thought as we re-write the pipelines. It's hard to tell which files are used by which pipelines when they're all smushed into the same tree.

# See the LICENSE file in the project root for more information. #
#################################################################################
parameters:
- name: approvalAliases
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be documenting all parameters. Nothing fancy, just a sentence or two about what a parameter does.

jobs:
- job: PublishPackages
displayName: 'Publish Packages'
dependsOn: AwaitApproval
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the approval step just live in this file? Is approval really a general task? It seems pretty specific to publishing packages.

buildType: current
artifactName: ${{ parameters.packageFolderName }}
targetPath: $(Pipeline.Workspace)/release/packages
- script: |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it also be helpful to list the contents of targetPath from the previous step?

dryRun: ${{ parameters.dryRun }}
internalFeedSource: ${{ parameters.internalFeedSource }}
packagesGlob: $(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg
- ${{ if eq(parameters.publishDestination, 'Public') }}:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer #{{ else }} ?

I would probably swap this to be:

${{ if eq(parameters.publishDestination, 'Public') }}:
  # Do public stuff.
${{ else }}:
  # Do non-public stuff.

parameters:
dryRun: ${{ parameters.dryRun }}
internalFeedSource: ${{ parameters.internalFeedSource }}
packagesGlob: $(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future consideration: We need to pick nicer names for these artifacts :)

default: '$(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg'

steps:
- script: |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we start putting these elaborate PowerShell scripts into files so we can:

  • Read them easily (syntax highlighting etc).
  • Run them (i.e. to test that they work).
  • Version them separately from the pipeline files.

default: '$(System.DefaultWorkingDirectory)/_dotnet-sqlclient-Official/drop_buildMDS_build_signed_package/*.nupkg'

steps:
- task: NuGetToolInstaller@1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be surprised if the images we use don't have NuGet installed already.

values:
- MDS
- MSS
- AKV
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AKV has its own official build/sign/release pipeline. Do we need it here?

@paulmedynski paulmedynski self-assigned this Nov 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants