-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generalize naming of common pipeline-related artifacts #3088
Conversation
Meant to be a link to dotnet/docker-tools#843 (vs. Is this PR in the right repo to change the common files? It does make sense to test the results in this repo though. |
Yes, thanks. I've fixed the link. I'm making these changes in the nightly branch here because I feel it gives better context to the changes to see what changes are necessary to the consuming pipelines which aren't in the docker-tools repo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't dug through the entire thing, but the bits being pulled out make sense to me. A few suggestions.
- name: acr.servicePrincipalPassword | ||
value: $(app-dotnetdockerbuild-client-secret) | ||
- name: acr.password | ||
value: $(BotAccount-dotnet-docker-acr-bot-password) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems kind of questionable to me to manually maintain this file in each .NET repo--would it make sense to add a common dotnet configuration template that repos can opt into? (Also for publicProjectName
/ internalProjectName
.)
One thing not being addressed in this PR is the usage of |
if ($env:SYSTEM_TEAMPROJECT -eq "internal") { | ||
$imageBuilderBuildArgs = "$imageBuilderBuildArgs --registry-override $(acr.server) --repo-prefix $(stagingRepoPrefix) --source-repo-prefix $(mirrorRepoPrefix) --push --registry-creds ""$(acr.server)=$(acr.userName);$(BotAccount-dotnet-docker-acr-bot-password)""" | ||
if ($env:SYSTEM_TEAMPROJECT -eq "${{ parameters.internalProjectName }}") { | ||
$imageBuilderBuildArgs = "$imageBuilderBuildArgs --registry-override $(acr.server) --repo-prefix $(stagingRepoPrefix) --source-repo-prefix $(mirrorRepoPrefix) --push --registry-creds ""$(acr.server)=$(acr.userName);$(acr.password)""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this fixes dotnet/docker-tools#835 too. (Well, sort of: it opens up a workaround-seeming fix that looks good enough to me!) For an internal build that I want to prevent from pushing tags to my ACR (like a PR validation build), I can make the pipeline file leave internalProjectName
as null
, so this never evaluates to true. 👍
if ("$(System.TeamProject)" -eq "$(publicProjectName)") { | ||
$basePath = "$(gitHubVersionsRepoInfo.path)" | ||
} | ||
else { | ||
$publicSourceBranch = "main" | ||
$basePath= "$(azdoVersionsRepoInfo.path)" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the file again, is this saying that internal builds can only publish image info to the AzDO/internal repo? Don't we want to publish it to the public dotnet/versions repo, as long as we aren't doing a truly private build? (We can't publish anywhere at all in public-project builds, right?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This template sets the variable for the purposes of reading the image info file. It's not relevant to the publishing of the image info file. For publish, it publishes to both the public and internal repos. It pushes the commit to both to avoid latency issues (see dotnet/docker-tools#642). For public project builds, it runs with the dry-run option, so nothing actually gets published.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right... and I guess having them different is just an error right now (https://github.com/dotnet/docker-tools/blob/2dddfe26ef6faca5f110fe1f6c179e47a51f3433/src/Microsoft.DotNet.ImageBuilder/src/Commands/PublishImageInfoCommand.cs#L35-L39) but this could be used in the future.
The common infrastructure located in the eng/common folder had names of projects, repositories, variables, branches, and paths that were specific to the .NET implementation. In order to make the infrastructure suitable for general purpose, these names need to be generalized.
app-dotnetdockerbuild-client-secret
=>acr.servicePrincipalPassword
).officialRepoPrefix
so that the pipeline logic can know whether a givenpublishRepoPrefix
value represents an official repo location.azdoVersionsRepoInfo.path
andgitHubVersionsRepoInfo.path
variable to provide a way to specify the directory path where image info files are located in those respective repos.publicSourceBranch
andadditionalPublishMcrDogsArgs
variables and reference a .NET-specific variable group in the publish stage.Related to dotnet/docker-tools#843