Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Docker image #631

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add Docker image #631

wants to merge 2 commits into from

Conversation

paroxp
Copy link
Member

@paroxp paroxp commented Nov 2, 2018

What

We'd like to add a docker image, in order to use the prototype kit as "well known" example application that can be hosted on (in our case) Kubernetes.

We had to work in a Docker argument in order for it to be used by the kit to either disable or enable the data usage sharing.

How to review

  • Review code
  • Run:
    docker build . -t "govuk-prototype-kit:0.0.$(date +"%s")"
    or
    docker build . -t "govuk-prototype-kit:0.0.$(date +"%s")" --build-arg COLLECT_USAGE_DATA=false
    This will build a Docker image you can run
  • Run:
    docker run -p 3000:3000 "govuk-prototype-kit:0.0.$(date +"%s")"
    Make sure the timestamp matches the one from the build step
  • You should be able to visit http://localhost:3000/

@joelanman
Copy link
Contributor

@paroxp thanks for this. Can you say a bit more about the user needs for this PR? And is this something that will benefit other teams?

@paroxp
Copy link
Member Author

paroxp commented Nov 5, 2018

Sure @joelanman.

In simplest terms:

The user need is being able to deploy prototype-kit based apps to platforms that support running as (Docker) containers. ie. Kubernetes, Docker Swarm, GOV.UK PaaS. As an added bonus it can also allow running the application locally on a laptop without having to install any additional dependencies other than Docker. - Currently Node is required following the npm install.

On the "Build/Run" teams we're investigating future platforms for GDS and many of them are Docker based... we would like to ensure that the prototype kit is well support and easy to deploy and so are using it as an example/hello-world type application to demonstrate what deployments to these platforms can look like.

Hope that helps. Do let me know if you have any other questions or concerns. I am also happy to have an in person chat :)

@aliuk2012
Copy link
Contributor

Again thanks for submitting this. 💯

I have reviewed your PR but could I suggest perhaps adding documentation on how to use the docker version to the readme or under the "advanced" installation pages (https://govuk-prototype-kit.herokuapp.com/docs/install/developer-install-instructions). This is just to make Developers aware that they could just use docker to install and run the prototype kit should they wish too.

Copy link
Contributor

@joelanman joelanman left a comment

Choose a reason for hiding this comment

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

Just chatted to Chris at GDS. Two main things:

  • which user groups need Docker support?
  • if we add it:
    • we might also need documentation on how to use it
    • it would be good to avoid defining a Node version both here and in package.json

As we'd like to support Docker, we're providing documentation on why and
when would an individual use it, as well as how to get around doing so.
@paroxp
Copy link
Member Author

paroxp commented Nov 22, 2018

  • Rebased on master
  • Documentation added - containing the user groups.

it would be good to avoid defining a Node version both here and in package.json

I know what you mean. However it's not a Node version in the Dockerfile per-se. It's a tagged operating system image with node pre-installed. Conveniently they match the Node versions.

I don't think I will be able to read the version from the package.json and parse it in for the docker build step. We could set it to something like: 8-alpine, carbon-alpine or simply latest (11.2 at a time).

ADD . /app
WORKDIR /app

ARG COLLECT_USAGE_DATA=true
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should probably be false by default

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it need a default value? Can we ask users the same as normal?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think because heroku bypasses this we've not run into it before, I ran into this same issue trying to get the kit running in Glitch, maybe there's a way we could timeout the question or have an environment variable, or something else to stop the server from stalling.

Copy link
Member Author

Choose a reason for hiding this comment

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

When pushing an image to the platform, interactive mod isn't a thing. Nor is by default on local environments.

We considered setting this to false, however we didn't want to step on your toes too much :)

Happy to set it to false.

@NickColley
Copy link
Contributor

This proposal might be able to be simplified now that #697 is merged

@zuzak zuzak mentioned this pull request Mar 12, 2019
@joelanman
Copy link
Contributor

@nataliecarey I'm thinking we close this? Is there a user need for Docker support?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants