refactor file to take better account of layers.

[krystan]

Made changes to the layout of the Dockerfile in typescript containers to make more efficient use of layers.

[msftclas]

All CLA requirements met.

[Chuxel]

Thanks for the contribution!

However, these files are intended to be educational for those less familiar with Dockerfiles rather than being optimized for deployment, so we'll definitely want the comments to remain so it is clear as to what is happening.

It's also important to have ENV DEBIAN_FRONTEND=dialog at the very end, since the point of including ENV DEBIAN_FRONTEND=noninteractive is to avoid errors appearing in the output log. See here.

Finally, to speed up the process of iterating on the dev container, we should keep rm -rf /var/lib/apt/lists/* separate. This allows developers to add their own content into the dockerfile between the primary "RUN" and this final cleanup step. The primary "RUN" player is then reused while the developer is iterating, which speeds up the entire process. We could do the autoremove/clean steps in the layer, but let's leave rm -rf /var/lib/apt/lists/* separate since running this command will force developers to run apt-get update again.

[krystan]

I have altered the front end statement, however, whilst I agree that the comments are useful, if they are intended for an audience that is not familiar with Docker, then perhaps also the layers should be optimal. People new to docker do not necessarily know this is an important thing that increases the size of the image.

Perhaps there is a way to include the comments but also correct the way in which the file is structured.

It is not just an optimisation but indeed it is listed as docker best practise on their website. And perhaps people newer to the technology are best served by following that best practice?

[Chuxel]

It looks like we should be able to layer in the comments within the single RUN statement. This works for typescript-node-8:

#-------------------------------------------------------------------------------------------------------------
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License. See https://go.microsoft.com/fwlink/?linkid=2090316 for license information.
#-------------------------------------------------------------------------------------------------------------

FROM node:8

ENV DEBIAN_FRONTEND=noninteractive
ENV SHELL /bin/bash

RUN apt-get update \
    #
    # Install apt-utils to avoid warnings
    && apt-get -y install --no-install-recommends apt-utils 2>&1 \
    #
    # Verify git and needed tools are installed
    && apt-get install -y \
        git \
        procps \
        curl \
        apt-transport-https \
        lsb-release \
    #
    # Remove outdated yarn from /opt and install via package 
    # so it can be easily updated via apt-get upgrade yarn 
    && rm -rf /opt/yarn-* \
    && rm -f /usr/local/bin/yarn \
    && rm -f /usr/local/bin/yarnpkg \
    && curl -sS https://dl.yarnpkg.com/$(lsb_release -is | tr '[:upper:]' '[:lower:]')/pubkey.gpg | apt-key add - 2>/dev/null \
    && echo "deb https://dl.yarnpkg.com/$(lsb_release -is | tr '[:upper:]' '[:lower:]')/ stable main" | tee /etc/apt/sources.list.d/yarn.list \
    && apt-get update \
    && apt-get -y install --no-install-recommends yarn \
    #
    # Install tslint and typescript globally
    && yarn global add \
        tslint \
        typescript \
    #
    # Clean up
    && apt-get autoremove -y \
    && apt-get clean -y \
    && rm -rf /var/lib/apt/lists/*

ENV DEBIAN_FRONTEND=dialog

[krystan]

Great I will make the changes as described, it's all good

[krystan]

changes have been made as suggested, inline comments placed back in, to enable people who are newer to docker to understand what is meant by certain operations.

[kmehant]

@Chuxel any thoughts about my points regarding this PR?

[kmehant]

It does not contribute to the purpose of using ENV DEBIAN_FRONTEND=noninteractive as you are trying to reassign immediately with dialog. We are using noninteractive because the following layers as per the original Dockerfile should run without user intervention.

[krystan]

I was not sure about this either

[kmehant]

It does not seem a necessary optimization as we have been using a standard template for creating Dockerfiles for dev containers. You can find the standard template in the repository and it seems very easy, maintainable and clean in future using the standard template for all the dev containers 😄

[kmehant]

Merging all the RUN commands into a single layer will be a good optimization. But, As mentioned earlier we are using a template for Dockerfiles, which means few layers are in common among different dev containers so this means we are actually optimizing the docker build process by dividing them into different RUN layers wherever necessary for better caching. So dev containers with similar layers will build faster due to cache but in your case and running all commands in a single intermediate container has least possibility to match other dev containers so making the build process slow.

[Chuxel]

I think as long as we can make it clear with comments and the "&&" being before the line so people can see that it is a continuation, we are probably okay here. The main concern is getting things so it is "cut-and-pasteable" into another definition. That was my main comment on that point here. It makes it clearer that the commands are a continuation of a broader command.

[kmehant]

Shifting this layer, as mentioned earlier does not contribute to the purpose.

[Chuxel]

@krystan So, I'm thinking that if we move forward with this, we'll probably want to do this largely across the board which fortunately isn't too hard -- but I want to get some regression testing scripts in place to make it a bit easier to retest things that are known to work. Looking thorugh @kmehant's comments, I think these are primarily are due to the fact that it looks like the JavaScript flavor didn't get updated like the TypeScript ones (since those already have the updates we discussed).

[krystan]

ok is that something you need some help with or should I leave this for now ?

[Chuxel]

So what I did was started a working branch for these: https://github.com/microsoft/vscode-dev-containers/tree/layer-consolidation

What might be the best thing to do would be to actually do a PR into this branch instead so we can make these changes w/o impacting releases if needed. Mainly worrying about unexpected regressions due to small typos, etc.

I did a pass but left the typescript / JS definitions alone so we can merge it in. That work?

//cc: @chrmarti as FYI