Add support for building ARM images#28
Conversation
|
@MichaelSimons, |
|
@dagood, would you have time to take a look at this? Thanks |
dagood
left a comment
There was a problem hiding this comment.
Some nits and a suggestion, but LGTM.
| architecture = Architecture.ARM; | ||
| break; | ||
| default: | ||
| throw new NotSupportedException("Unknown Docker Architecture"); |
There was a problem hiding this comment.
Seems like a good place for PlatformNotSupportedException.
| { | ||
| string architecture = GetArgValue(args, ref i, "architecture"); | ||
| options.Architecture = (Architecture)Enum.Parse(typeof(Architecture), architecture, true); | ||
|
|
| } | ||
|
|
||
| public static RepoInfo Create(Repo model, Manifest manifest, string dockerOS, string includePath) | ||
| public static RepoInfo Create(Repo model, Manifest manifest, Options options, string dockerOS) |
There was a problem hiding this comment.
I'm a little concerned with passing in Options... it has a ton of stuff that isn't needed to create the view model. Looks fine as a quick way to pass in everything needed, but could be better I think.
There was a problem hiding this comment.
I was hesitant to do this in the first place - reverted back to passing individual params.
There was a problem hiding this comment.
My thought would be to have a ManifestInfoFilter data object that Options fills in, or that gets generated from Options properties.
This required a schema change to the manifest. The following is an example of the new schema used in the dotnet-docker-nightly repo.
Related #19