Allow building and using images for Windows Server 2022 as well as 2019 #212
Conversation
Codecov Report
@@ Coverage Diff @@
## master #212 +/- ##
=======================================
Coverage 80.50% 80.50%
=======================================
Files 14 14
Lines 785 785
=======================================
Hits 632 632
Misses 94 94
Partials 59 59 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
looks good to me! |
There was a problem hiding this comment.
❤️🙏☕️ Thank you so much for adding me to this review, looks good, and I hope you have tested this from your repo :) 🐈🐈 which seems like the only way for sanity check.
Had some thoughts, idea, and information to share hence commented. Rest looks good to me. Thanks.
| run: | | ||
| echo "Version: ${{ steps.changelog_reader.outputs.version }}" | ||
| echo "Changes: ${{ steps.changelog_reader.outputs.changes }}" | ||
| $owner = "${{ github.repository_owner }}".ToLower() |
There was a problem hiding this comment.
💡observation/question - In bash I think assignment of variables with space after the (=) sign doesn’t work, noticed spaces here $owner = "${{ github.repository_owner }}".ToLower() - Is it because now we use "PowerShell --> under ubuntu-latest"?
There was a problem hiding this comment.
Yeah, exactly. These workflows have jobs that run on different platforms, and using PowerShell is the simplest way to make the scripting syntax the same for all of them. For consistency, I thought it'd be best for all the jobs to use PowerShell, even if they didn't need to.
Also, although PowerShell doesn't care whether there's a space or not, I quite like having it there because it makes it (slightly) more obvious it's not bash, and might save us in future from adding bash syntax and wondering why it isn't working...
| run: | | ||
| echo "Version: ${{ steps.changelog_reader.outputs.version }}" | ||
| echo "Changes: ${{ steps.changelog_reader.outputs.changes }}" | ||
| $owner = "${{ github.repository_owner }}".ToLower() |
There was a problem hiding this comment.
💡☕️ Idea only: Thinking out loud and sharing, shall we make this Env Var https://docs.github.com/en/actions/learn-github-actions/variables#defining-environment-variables-for-a-single-workflow
There was a problem hiding this comment.
In this case I probably wouldn't bother, because it's just local to that particular step, and not shared anywhere else. But that's a good point - I didn't think to consider how we might be able to improve our workflow files with env vars.
| $tagbase = "${{ needs.common.outputs.tagbase }}" | ||
| az acr login -n ${{ vars.AZURE_REGISTRY_SERVER }} | ||
| docker manifest create $tagbase ${{ needs.publish.outputs.linux }} ${{ needs.publish.outputs.win2019 }} ${{ needs.publish.outputs.win2022 }} | ||
| docker manifest push $tagbase |
There was a problem hiding this comment.
💡 for historic Archival reasons: this command is experimental for now: https://docs.docker.com/engine/reference/commandline/manifest_push/
There was a problem hiding this comment.
Interesting! I didn't notice that. I used it because it was documented here as the recommended way to publish multi-arch images. I'm prepared to take a punt on this functionality not being removed, and handle it if the command/syntax changes.
This addresses #208.
To avoid needing different DaemonSets for each Windows version, the resource specs have been changed to use multi-platform images. In other words, a single image tag refers to an image for whatever platform the container is deployed to.
This requires creating a manifest that associates the multi-platform image tag with each of the platform-specific tags, so both the GHCR and MCR workflows have been updated to publish those.
The Dockerfiles have also been updated to use lightweight golden MCR images as base images. The Linux version now uses the distroless Mariner base image, and the Windows versions are based on the lighter-wight Nanoserver, reducing image pulling time.
Note: the MCR workflow configuration (ACR and Azure app identity) is moved from GH secrets to variables. This was due to some refactoring in which it made sense for job outputs to include the ACR, but that was failing because GitHub sensibly doesn't allow secret values to be passed around this way. It would've been possible to just store the ACR name as a variable and keep the rest as secrets, but none of those values are really confidential and it's simpler to have them in the same place. It's probably worth having someone double-check my reasoning though. We'll have to update those variables in the Azure fork before the next MCR deployment.