Beginnings build container sample. by glennc · Pull Request #1 · dotnet/dotnet-docker-samples

glennc · 2016-10-11T00:31:13Z

This still needs work, but enough is there to show what I was thinking with this sample. We would add a nano build image and a powershell script to finish it off.

What do you think?

@MichaelSimons @richlander

richlander · 2016-10-16T05:14:20Z

+
+rm -r ./out
+
+if [ -n "$currentBuildImage" ]; then


Why persist this state and then wait until the end to do the cleanup? Why not just do it up front?

Can you explain what the goal of this cleanup logic is? On a clean machine, run the script twice back to back. It appears that on the second run, the cleanup logic ends up deleting the images that were just built. Additionally the script errors out if there is a container that is referencing the app image.

You want the layers available when you are building the next image so that you don't do NuGet restore again. But after you are done you will have tagged the new build image and the old one will have no tag. So we use the tag to identify an image that after the build will be untagged and should be cleaned up. Does that make sense now?

The app image I just threw in because it was a sample and it seemed reasonable to delete it instead of leaving it untagged. I could add -f to avoid errors and do the delete but that seemed a bit aggressive.

or am I missing something that makes this unnecessary?

@glennc - I understand what you are trying to achieve. My concern is there seems to be some edge cases that aren't handled, for example:

If you run the script without changing the program after running it once already, then the app image doesn't get rebuilt because the layers haven't changed (this also occurs for the build image). The end result is the cleanup logic is deleting the previous image which is also the current image.

As I mentioned if you run the app image and don't cleanup the container, then you will get an error from the cleanup logic.

While I think it is important to showcase how you should cleanup the Docker artifacts. If we don't handle the these edge cases that I think users could easily get into, then I think it detracts from the sample.

richlander · 2016-10-16T05:16:04Z

LGTM

The readme needs to be updated, still. That can be done after the merge. Same with the batch/ps script for Windows.

Great sample!

Thoughts @MichaelSimons ?

MichaelSimons · 2016-10-17T15:41:02Z

@@ -0,0 +1 @@
+project.lock.json


Feels like this may better belong at the root.

MichaelSimons · 2016-10-17T15:42:29Z

several of the files are missing a newline at the end of the file.

MichaelSimons · 2016-10-17T15:43:55Z

@@ -0,0 +1,4 @@
+FROM microsoft/dotnet:core


microsoft/dotnet:core [](start = 5, length = 21)

I think our samples should be using an explicit version as showcasing the best practices and avoiding the risk of breaking.

Sure, I'll change it. IIRC I thought I still had a lot of work to do on this one so I hadn't done a pass looking for this sort of stuff.

MichaelSimons · 2016-10-17T15:47:15Z

+
+# Add the rest of my source files.
+COPY . .
+RUN dotnet publish --output /out --configuration Release


--configuration [](start = 33, length = 15)

I think we should be consistent across our samples. Rich is using -c in the samples he created.

Sure thing, I'll change that.

MichaelSimons · 2016-10-17T15:56:05Z

+# Copy the contents of the /out directory out so that we can build our app image.
+docker cp build-container:/out ./out
+# Build the application image.
+docker build -t dockerapp -f ./out/Dockerfile ./out


If I follow what is happening correctly, the Dockerfile is getting copied to the dockerapp image. That doesn't seem optimal.

You could just put it in the project.json and you wouldn't need to copy it. I had meant to make that change, I was experimenting with how I could do it without.

I'm not following how that is going to prevent the Dockerfile from being added to the dockerapp image. I was thinking you would need to do something like, stop coping the Dockerfile to the out directory and build via docker build -t dockerapp . Lastly the Dockerfile would have to be modified to copy from out - a74d917

I misunderstood what you were asking about. For some reason I thought you were asking about copying the Dockerfile into the publish output folder, not adding it to the image. Yes I can change that.

…re consistent, use explicit version in the Dockerfile, and stop the Dockerfile from being part of the final image.

glennc · 2016-10-18T23:37:18Z

@richlander @MichaelSimons How does that look now? Added some windows support...

richlander · 2016-10-19T05:08:52Z

@kendrahavens Want to take a look?

MichaelSimons · 2016-10-19T13:54:00Z

+
+param(
+    [ValidateSet('win', 'linux')]
+    [string]$baseImage = "win"


win [](start = 26, length = 3)

We should be consistent across the samples on the terminology used for the nano images. The other samples are using nano.

ok, sure thing.

MichaelSimons · 2016-10-19T13:55:06Z

+
+# Capture the current build and app image, if any, so that we can clean them up when we are done.
+$currentBuildImage=$(docker images build-image -q)
+$currentAppImage=$(docker images dockerapp -q)


Same comment as with the build.sh - what is the goal of this cleanup logic?

MichaelSimons · 2016-10-19T14:05:54Z

{

I just noticed an inconsistency with the other sample apps. They are explicitly named - e.g. "name": "dotnetapp", I honestly have no preference other than we are consistent. This obviously affects the prod containers.

Refers to: dotnetapp-buildcontainer/project.json:1 in 315cd4e. [](commit_id = 315cd4e, deletion_comment = False)

MichaelSimons · 2016-10-19T14:15:52Z

@@ -0,0 +1,38 @@
+
+param(


I like that you are utilizing xplat powershell!

MichaelSimons · 2016-10-19T23:50:37Z

@@ -0,0 +1,27 @@
+#!/bin/sh


For both script files, what are your thoughts on handling error cases. I am primarily thinking about the experience when there is a build error in the app that is getting built.

This really comes down to how complete of a sample are we trying to build and what are we trying to showcase? Is this sample something we would want devs to be able to copy into their workflow with relatively few changes? Do we want to keep this sample relatively simple in order to better showcase the flow that can be achieved?

MichaelSimons · 2017-04-19T23:30:15Z

This problem is solved by the multi-step build infrastructure Docker now has. This PR is replaced with #44.

richlander reviewed Oct 16, 2016

View reviewed changes

MichaelSimons reviewed Oct 17, 2016

View reviewed changes

glennc added 2 commits October 18, 2016 12:04

Beginnings build container sample.

c912ff3

Respond to feedback: move dockerignore to root, make publish usage mo…

b40d79e

…re consistent, use explicit version in the Dockerfile, and stop the Dockerfile from being part of the final image.

glennc force-pushed the glennc-buildContainer branch from 3b5c898 to b40d79e Compare October 18, 2016 19:05

Add windows container support

315cd4e

MichaelSimons reviewed Oct 19, 2016

View reviewed changes

aaronjones mentioned this pull request Feb 21, 2017

Unable to get dotnetapp-prod working #31

Closed

MichaelSimons closed this Apr 19, 2017

Conversation

glennc commented Oct 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richlander commented Oct 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelSimons commented Oct 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glennc commented Oct 18, 2016

Uh oh!

richlander commented Oct 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelSimons commented Oct 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelSimons commented Apr 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

richlander commented Oct 16, 2016 •

edited

Loading