Skip to content
This repository has been archived by the owner on Nov 30, 2023. It is now read-only.

refactor file to take better account of layers. #70

Closed
wants to merge 7 commits into from
Closed

refactor file to take better account of layers. #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
27600c4
refactor file to take better account of layers.
krystan Jun 7, 2019
0848096
moved front end to the end where it should be
krystan Jun 7, 2019
0aa8765
put commends back inline to help demonstrate what commands mean
krystan Jun 8, 2019
7db43f8
placed comments back in and continue with layers that are more optima…
krystan Jun 8, 2019
963ef57
added the word 'globally' to better clarify the npm install statement…
krystan Jun 8, 2019
bd66218
changed apt update to inline in run statement
krystan Jun 8, 2019
ba1060e
corrected positioning of comment
krystan Jun 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 10 additions & 21 deletions containers/javascript-node-lts/.devcontainer/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,33 +5,22 @@

FROM node:lts

# Configure apt
ENV DEBIAN_FRONTEND=noninteractive
RUN apt-get update \
&& apt-get -y install --no-install-recommends apt-utils 2>&1

# Verify git and needed tools are installed
RUN apt-get install -y git procps
ENV DEBIAN_FRONTEND=dialog
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not sure about this either

ENV SHELL /bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄


# Remove outdated yarn from /opt and install via package
# so it can be easily updated via apt-get upgrade yarn
RUN rm -rf /opt/yarn-* \
RUN apt-get update \
&& apt-get -y install --no-install-recommends apt-utils 2>&1 \
&& apt-get install -y git procps \
&& rm -rf /opt/yarn-* \
Copy link
Contributor

@kmehant kmehant Jun 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

&& rm -f /usr/local/bin/yarn \
&& rm -f /usr/local/bin/yarnpkg \
&& apt-get install -y curl apt-transport-https lsb-release \
&& 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 eslint
RUN npm install -g eslint

# Clean up
RUN apt-get autoremove -y \
&& apt-get -y install --no-install-recommends yarn \
&& npm install -g eslint \
&& apt-get autoremove -y \
&& apt-get clean -y \
&& rm -rf /var/lib/apt/lists/*
ENV DEBIAN_FRONTEND=dialog
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


# Set the default shell to bash instead of sh
ENV SHELL /bin/bash
&& rm -rf /var/lib/apt/lists/* \
34 changes: 16 additions & 18 deletions containers/typescript-node-8/.devcontainer/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,33 +5,31 @@

FROM node:8

# Configure apt
ENV DEBIAN_FRONTEND=noninteractive
RUN apt-get update \
&& apt-get -y install --no-install-recommends apt-utils 2>&1

# Verify git and needed tools are installed
RUN apt-get install -y git procps
# Set the default shell to bash instead of sh
ENV SHELL /bin/bash

# Remove outdated yarn from /opt and install via package
# so it can be easily updated via apt-get upgrade yarn
RUN rm -rf /opt/yarn-* \
# Configure apt
RUN apt-get update \
&& apt-get -y install --no-install-recommends apt-utils 2>&1 \
# Verify git and needed tools are installed
&& apt-get install -y git procps \
# 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 \
&& apt-get install -y curl apt-transport-https lsb-release \
&& 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
RUN npm install -g tslint typescript

# Clean up
RUN apt-get autoremove -y \
&& apt-get -y install --no-install-recommends yarn \
# Install tslint and typescript globally
&& npm install -g tslint typescript \
# Clean up
&& apt-get autoremove -y \
&& apt-get clean -y \
&& rm -rf /var/lib/apt/lists/*
ENV DEBIAN_FRONTEND=dialog

# Set the default shell to bash instead of sh
ENV SHELL /bin/bash
ENV DEBIAN_FRONTEND=dialog
34 changes: 16 additions & 18 deletions containers/typescript-node-lts/.devcontainer/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,33 +5,31 @@

FROM node:lts

# Configure apt
ENV DEBIAN_FRONTEND=noninteractive
RUN apt-get update \
&& apt-get -y install --no-install-recommends apt-utils 2>&1

# Verify git and needed tools are installed
RUN apt-get install -y git procps
# Set the default shell to bash instead of sh
ENV SHELL /bin/bash

# Remove outdated yarn from /opt and install via package
# so it can be easily updated via apt-get upgrade yarn
RUN rm -rf /opt/yarn-* \
# Configure apt
RUN apt-get update \
&& apt-get -y install --no-install-recommends apt-utils 2>&1 \
# Verify git and needed tools are installed
&& apt-get install -y git procps \
# 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 \
&& apt-get install -y curl apt-transport-https lsb-release \
&& 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
RUN npm install -g tslint typescript

# Clean up
RUN apt-get autoremove -y \
&& apt-get -y install --no-install-recommends yarn \
# Install tslint and typescript globally
&& npm install -g tslint typescript \
# Clean up
&& apt-get autoremove -y \
&& apt-get clean -y \
&& rm -rf /var/lib/apt/lists/*
ENV DEBIAN_FRONTEND=dialog

# Set the default shell to bash instead of sh
ENV SHELL /bin/bash
ENV DEBIAN_FRONTEND=dialog