-
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
Enable testing of internal servicing builds #5928
base: main
Are you sure you want to change the base?
Conversation
@NikolaMilosavljevic, could you please link to a test run or something that illustrates the generated Dockerfiles? |
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.
Can you describe the overall workflow for how this will work? It seems that these changes rely on the Dockerfiles already being regenerated to target internal drops. How will that happen in an automated way? I'd like to understand the overall plan before proceeding with these changes.
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.
The BlobStorageSasQueryString
parameter is obsolete now and should be removed. This applies more broadly too. Do a search for "SasQueryString" in the repo and you'll find a lot of occurrences which all need to get cleaned up since this new implementation is replacing that.
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.
Fixed with f9700ce
httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue( | ||
"Basic", | ||
Convert.ToBase64String(Encoding.ASCII.GetBytes(string.Format("{0}:{1}", "", | ||
Config.NuGetFeedPassword)))); |
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.
Config.NuGetFeedPassword
should be renamed to better fit what its use is for. This is not a NuGet feed we're accessing here.
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.
Correct - I'm overloading the usage of this property. I was thinking of adding a new one for artifacts feed.
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.
Fixed with f9700ce
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.
Be aware that all of the changes made in the eng/common directory need to be applied to https://github.com/dotnet/docker-tools. That is the primary repo for those files and they then get flowed to other repos, including this one.
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.
Will create a separate PR for docker-tools
. Would it be ok if I keep the changes in this PR, as well? Flow from the other repo would overwrite them anyway.
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.
Sure, it's fine to keep these changes here. Just be aware that you'll need to apply these changes to docker-tools in a prompt manner; otherwise, they'll get overwritten.
Test run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2548267&view=results |
I'm adding a paragraph to the PR description. In essence, to avoid adding additional complexity with updating dependencies in each leg, we've decided to run dependency (and dockerfiles) update in a separate pipeline that would commit those changes in a new branch and trigger the build/test pipeline. |
How will we ensure regressions aren't introduced into the templates for this internal testing scenario with "normal" changes going forward? Is there a way to validate in PR validation? At a minimum we should ensure that the internal servicing validation pipeline is run anytime there are impactful infrastructure changes made (e.g. Dockerfile template changes) in order to flush out regressions right away versus in the release endgame when we are trying to validate product builds. |
That is a concern and something needs to be done. We've had this issue already it was on to developer to ensure template changes do not regress any of the scenarios. I've created an issue to track any verification work: #5935 |
…d in internal testing
Contributes to: https://github.com/dotnet/dotnet-docker-internal/issues/5952
This PR includes changes in 4 distinct areas, with its own commits:
Summary of changes:
Workflow
This pipeline runs build and test on a branch that contains the updated dockerfiles (that reference internal build artifacts). Another pipeline will be implemented, to run the dependency (and dockerfile) update step, commit changes to a feature branch, named appropriately (i.e. include BuildId in the name), and push the branch. Trigger would be added to initiate the run of the build/test pipeline as soon as new testing branch is pushed.