search

dotnet / docker-tools

This is a repo to house some common tools for our various docker repos.
MIT License
124 stars 46 forks source link

Specify ACR for Copying Images #1295

Closed ellahathaway closed 5 months ago

ellahathaway commented 6 months ago

Related to https://github.com/dotnet/dotnet-docker-internal/issues/5024 and https://github.com/dotnet/dotnet-docker-internal/issues/5026

This PR updates the CopyAcrImages command to include a SourceRegistry argument. This argument is necessary for copying the images from the staging directory to the publish directory. The corresponding tests have also been updated.

Test pipeline run

lbussell commented 5 months ago

@ellahathaway you can rebase on https://github.com/dotnet/docker-tools/pull/1297 and begin testing. I'll be pushing some additional commits to fix the pipelines. If you want to get started now, you can just continue rebasing as I apply the updates.

ellahathaway commented 5 months ago

IK that @MichaelSimons had some offline questions about the pipeline changes, so should I wait to merge this until we close out on that discussion?

MichaelSimons commented 5 months ago

IK that @MichaelSimons had some offline questions about the pipeline changes, so should I wait to merge this until we close out on that discussion?

I don't think waiting is necessary on this PR. They should be answered before https://github.com/dotnet/docker-tools/pull/1309 is merged.

ellahathaway commented 5 months ago

Had to adjust the PR in https://github.com/dotnet/docker-tools/pull/1295/commits/1e6dcfd342970af0bf577f54c4a0b38f837519ff to use the srcResourceId. This was necessary to fix a cred issue that arose after I made changes in https://github.com/dotnet/docker-tools/pull/1309 to use a different resource group and subscription for the new ACR.

ellahathaway commented 5 months ago

New test run (internal Microsoft link)

lbussell commented 5 months ago

@mthalman are we good to merge this now? I think there is enough time left until next Patch Tuesday to iron out any potential issues (or revert).

mthalman commented 5 months ago

Yeah, we should be good now.