fix: fix an issue causing container deployments to fail when run on an ARM-based system #861
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #861 +/- ##
==========================================
+ Coverage 62.36% 62.39% +0.02%
==========================================
Files 279 279
Lines 10900 10905 +5
Branches 1513 1515 +2
==========================================
+ Hits 6798 6804 +6
+ Misses 3566 3564 -2
- Partials 536 537 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
philasmar
force-pushed
the
asmarp/fix-docker-on-arm
branch
5 times, most recently
from
62dbba8 to
86e102f
Compare
| { | ||
| var template = Assembly.GetExecutingAssembly().ReadEmbeddedFile(DockerfileTemplate); | ||
| string templateLocation; | ||
| switch (targetFramework) |
There was a problem hiding this comment.
I am specifically checking the following target framework to maintain consistency with other parts of the codebase that also have the same check for different purposes.
| @@ -0,0 +1,26 @@ | |||
| FROM {docker-base-image} AS base{non-root-user} | |||
There was a problem hiding this comment.
The fix to container deployments only applies to .NET 7 and newer, so this dockerfile template is the version we currently use and will continue to be used for .NET6 and older targets.
| @@ -7,10 +7,6 @@ | |||
| <RootNamespace>AWS.Deploy.Orchestration</RootNamespace> | |||
| </PropertyGroup> | |||
|
|
|||
| <ItemGroup> | |||
There was a problem hiding this comment.
I added a project reference to AWS.Deploy.Recipes.CDK.Common so this reference is no longer needed.
| @@ -74,6 +74,11 @@ public async Task BuildDockerImage(CloudApplication cloudApplication, Recommenda | |||
| DockerUtilities.TryGetAbsoluteDockerfile(recommendation, _fileManager, _directoryManager, out var dockerFile); | |||
|
|
|||
| var dockerBuildCommand = $"docker build -t {imageTag} -f \"{dockerFile}\"{buildArgs} ."; | |||
| if (RuntimeInformation.OSArchitecture != Architecture.X64) | |||
There was a problem hiding this comment.
We don't currently support running tests on non X64 architectures, so this code path will not be tested in our CI. However, this code path is covered using existing tests when run on ARM.
| @@ -67,7 +67,7 @@ public void GenerateCDKProjectFromTemplate(Recommendation recommendation, Orches | |||
| var templateParameters = new Dictionary<string, string> { | |||
| // CDK Template projects can parameterize the version number of the AWS.Deploy.Recipes.CDK.Common package. This avoid | |||
| // projects having to be modified every time the package version is bumped. | |||
| { "AWSDeployRecipesCDKCommonVersion", FileVersionInfo.GetVersionInfo(typeof(Constants.CloudFormationIdentifier).Assembly.Location).ProductVersion | |||
| { "AWSDeployRecipesCDKCommonVersion", FileVersionInfo.GetVersionInfo(typeof(AWS.Deploy.Recipes.CDK.Common.CDKRecipeSetup).Assembly.Location).ProductVersion | |||
There was a problem hiding this comment.
Since we dropped Nerdbank.GitVersioning, this code was bugged as it always returned 1.0.0. This is because Constants.CloudFormationIdentifier is part of AWS.Deploy.Orchestration which is not versioned. This is why I added a project reference to AWS.Deploy.Recipes.CDK.Common and used AWS.Deploy.Recipes.CDK.Common.CDKRecipeSetup which is versioned by AutoVer.
…n ARM-based system
philasmar
force-pushed
the
asmarp/fix-docker-on-arm
branch
from
86e102f to
4299917
Compare
Issue #, if available:
DOTNET-7715
Description of changes:
Dockerfiletemplate to include a fix for the hangingdotnet restorein docker build. This follows the following Microsoft blog https://devblogs.microsoft.com/dotnet/improving-multiplatform-container-support/. This fix only applies to .NET7 and newer since the-aargument was added in .NET7.Dockerfiletemplate which maintains the current behavior for .NET6 and older.AWS.Deploy.Recipes.CDK.Commonis not used in the CDK projects which was broken after we droppedNerdback.GitVersioning. This issue does not currently exist in production. It is localized to thedevbranch.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.