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

I can develop the latest changelog.com version locally, with a single command: docker-compose up #246

Merged
merged 29 commits into from Oct 9, 2018

Conversation

gerhard
Copy link
Member

@gerhard gerhard commented Jul 5, 2018

This builds on #120 - thanks @sobolevn!

  • learn from @nickjj docker-web-framework-examples
  • single command, docker-compose up (make contrib if you are on macOS or Linux)
  • mount dev files from disk instead of COPY
  • mix run priv/repo/seeds.exs
  • proxy container for redirection rules & http://localhost
  • embed legacy assets into proxy image
  • update README (combine Can I contribute? with How do I run the code?)
  • works on Linux

@gerhard gerhard mentioned this pull request Jul 5, 2018
@nickjj
Copy link
Collaborator

nickjj commented Jul 5, 2018

If you use my example app as a guide, I would stick to that example's Dockerfile and entrypoint scripts. Each line exists for a purpose.

I also have the Changelog codebase running in Docker successfully. I haven't opened a PR because I had some issues with your Webpack set up (was getting a ton of errors, but I don't know enough about Webpack 4 and your code base to debug them).

I would most certainly separate the Phoenix process and Webpack watcher into their own containers too.

@gerhard gerhard changed the title WIP: Dev changelog.com in Docker 🛠️ I can develop the latest changelog.com version locally, with a single command Jul 6, 2018
@gerhard gerhard changed the title 🛠️ I can develop the latest changelog.com version locally, with a single command 🛠️ I can develop the latest changelog.com version locally, with a single command: gmake code Jul 6, 2018
@gerhard
Copy link
Member Author

gerhard commented Jul 20, 2018

Can y'all let me know if this works for you? @jerodsanto @adamstac @codyjames @sobolevn

It would be especially helpful to know how well this works on Windows. Will test on Linux next. Tested on Elementary OS, works as expected.

@gerhard gerhard changed the title 🛠️ I can develop the latest changelog.com version locally, with a single command: gmake code 🛠️ I can develop the latest changelog.com version locally, with a single command: docker-compose up Jul 20, 2018
RUN export DEBIAN_FRONTEND=noninteractive \
&& apt-get update \
&& apt-get install -y ffmpeg \
&& which ffmpeg \

Choose a reason for hiding this comment

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

Do you need this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. The app calls ffmpeg which will fail without this line. This might not matter as much in development, but I want to make sure that this works locally before we start building production images. @jerodsanto was mentioning about eventually removing this dependency, but until that happens, this can act as a reminder.

Makefile Outdated
echo "Database was not re-created with seed data"; \
fi

proxy: docker ## Builds & publishes thechangelog/proxy image

Choose a reason for hiding this comment

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

I guess all these targets could be marked as .PHONY

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea.

expose:
- "4000"
volumes:
- ./assets/admin/:/app/assets/admin/

Choose a reason for hiding this comment

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

Just wondering why there are so many individual volumes?

Copy link
Member Author

@gerhard gerhard Jul 20, 2018

Choose a reason for hiding this comment

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

Because I couldn't think of an easier way of defining the subset of ./assets paths that we want to mount.

Can you figure out a better way of mounting these paths:

./assets/admin
./assets/app
./assets/assets
./assets/email
./assets/semantic
./assets/shared
./assets/test
./assets/.babelrc
./assets/embedder.js
./assets/postcss.config.js
./assets/webpack.config.js

And not mounting these paths?

./assets/package-lock.json
./assets/package.json
./assets/yarn.lock

Choose a reason for hiding this comment

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

I have two questions about this setup.

  1. Why do you support both package-json.lock and yarn.lock? In my experiense this could lead to problems since they resolve the dependecies differently. And there can be different issues with that. https://stackoverflow.com/questions/44552348/should-i-commit-yarn-lock-and-package-lock-json-files

  2. Why do not you want to mount these three files?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. yarn replaced npm in replace npm with yarn to install dependencies. Ref #131 #132 and the package-json.lock never got removed. All gone now, thanks!
  2. These files are part of the Docker image itself. If they change, the expectation is that the image will be re-built via docker-compose build. This commit message has more context 2667ce3

Can you think of a better way?

Copy link

@sobolevn sobolevn Jul 23, 2018

Choose a reason for hiding this comment

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

Oh, I see. I still do not see a reason not to mount them, because sometimes you can change package.json without changing dependencies. There are settings, meta data and so on.

I guess it more a documentation thing. That's how we handle it.
Dockerfile: https://github.com/wemake-services/wemake-vue-template/blob/master/template/docker/vue/Dockerfile
docker-compose.yml: https://github.com/wemake-services/wemake-vue-template/blob/master/template/docker-compose.yml

Please, take a note that we do not mount node_modules, because it has to be built inside the container.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will mount everything and capture the exact errors which I was seeing initially. It's possible that once I implement learnings from your Dockerfile & compose, the errors will go away.

Thanks!

@gerhard
Copy link
Member Author

gerhard commented Oct 5, 2018

This works, I'm keen on wrapping it up - it's been running for too long.

The next step is to make it right by having @jerodsanto feedback all the things which are wrong with this for his dev workflow 😉

Eventually, the plan is to make it fast on Docker for Mac 🤦‍♂️ 🤦‍♀️

How to check if this works & is good?

cd changelog.com
git pull && git checkout docker-dev
make contrib

Change some files localy & see if http://localhost:4000 reacts as expected

@jerodsanto
Copy link
Member

When I ran make contrib it gave me the "install make 4 error".

When I ran gmake contrib it gave me this compilation error:

2018-10-08 at 11 33 am

(It appears that the code being compiled was referencing an outdated module in the Bamboo dependency.)

I then ran gmake build and that completed successfully.

I then ran gmake contrib and everything worked as expected. 👍

Should we address that compilation error somehow? If it's transient, I'd say this is good to merge.

@gerhard
Copy link
Member Author

gerhard commented Oct 8, 2018

make may not be the correct version on macOS, depending on how it was installed (see brew info make). On macOS, I alias m=gmake, and on Linux I alias m=make so I never have this problem. It's hard to come up with a default that would work for everyone. I'm proposing that we change the docs to gmake when we have a few more reports that make is tripping people up.

The contrib failure is a good catch. If there is an existing app image which has different dependencies than what is defined in the mix files, contrib will fail. The cached image will be re-created when build gets called since the layers have changed. What do you think about outputting a message to run build if contrib fails?

Another alternative to contrib failing when deps diverge is to always run build. This will result in unnecessarily downloading and compiling dependencies, which is what we used to have until a few commits ago. I thought that a third target which combines build & run would be confusing, but I'm happy to re-consider.

@jerodsanto
Copy link
Member

What do you think about outputting a message to run build if contrib fails?

I think that's a good way to go.

Run it with `docker-compose build app && docker-compose up`

It's still WIP and rough, but it's a step in the same direction as #120

Next steps:

* learn from https://github.com/nickjj/docker-web-framework-examples/tree/master/phoenix
* volumes instead of COPY
* db seed (clone?)
* one-liner (e.g. gmake dev)
* nginx
* dns resolution (e.g. changelog.local)

ping @jerodsanto @nickjj
You can also build it explicitly by running `gmake build`, and also just
`gmake run` if it's already been built, meaning that all dependencies
have already been cached in the Docker image.
Otherwise editing these files locally, on OS X, will not be reflected in
the running app container.

Notice that mix.exs, mix.lock, assets/package* as well as
assets/yarn.lock have not been mounted into the app container. If they
change, the container needs to be rebuilt since the compilation stage
happens **before** beam & webpack get booted. Any mix.exs changes will
require the BEAM VM to be restarted, so let's go with the simplistic
approach of requiring a full re-build if any of these files change. This
might be of interest to @nickjj, since this is a different take on the
"runtime lock stomping" problem:
nickjj/docker-web-framework-examples@48428b8#diff-73d444f37ad1743a8ae6f58d85ca5047
Add all redirect rules, including legacy ones

db and app containers are not available directly. This is on purpose,
only exposing the proxy for now, available via http://localhost
It has all legacy assets baked in, because they never change, and it
made more sense to keep them embedded here.
Old skool, I know...
Docker was not being installed correctly
Since they will be embedded in the app image, we want the app to always
serve these, not the proxy.

Static assets are moving from this model:

                                                    serve static asset
                                                    ^
                                                    |
    compiled assets volume - (read static asset) -> proxy (nginx)

To this model:

    serve static asset
    ^
    |
    proxy (nginx)
    ^
    |
    (proxy request)
    |
    app (embeds static assets)

We are making the app available on http://localhost:4000 so that devs
can keep the same development workflow.

Proxy is still available on http://localhost, even though static assets
are not being proxied correctly due to Access-Control-Allow-Origin.
Planning on addressing it via
https://medium.com/@yagoazedias_75857/how-to-configure-cors-in-your-phoenix-application-5ef0234bc25f

[#158222089]
They rarely change, it makes sense to include all nginx config in the
proxy image that will be eventually used in production.

[#158222089]
Learning: use Docker for Linux to have 0.08s response times

[#158222089]
This target keeps forcing upstream targets to be re-created, and it
doesn't add a lot of value. If Docker isn't running, this helpful error
gets displayed by default:

    ERROR: Couldn't connect to Docker daemon. You might need to start Docker for Mac.

[#158222089]
Not useful enough to be worth including it - especially in light of
the upcoming Linux support
A dev db copy will be more useful. Seeding the db is currently failing
with:

    ** (FunctionClauseError) no function clause matching in NaiveDateTime.to_erl/1

        The following arguments were given to NaiveDateTime.to_erl/1:

            # 1
            #Ecto.DateTime<2018-10-05 14:40:02>

        Attempted function clauses (showing 1 out of 1):

            def to_erl(%{calendar: _} = naive_datetime)

        (elixir) lib/calendar/naive_datetime.ex:597: NaiveDateTime.to_erl/1
        (arc_ecto) lib/arc_ecto/type.ex:54: Arc.Ecto.Type.dump/2
        (ecto) lib/ecto/type.ex:676: Ecto.Type.process_dumpers/3
        (ecto) lib/ecto/repo/schema.ex:785: Ecto.Repo.Schema.dump_field!/6
        (ecto) lib/ecto/repo/schema.ex:798: anonymous fn/6 in Ecto.Repo.Schema.dump_fields!/5
        (stdlib) lists.erl:1263: :lists.foldl/3
        (ecto) lib/ecto/repo/schema.ex:796: Ecto.Repo.Schema.dump_fields!/5
        (ecto) lib/ecto/repo/schema.ex:745: Ecto.Repo.Schema.dump_changes!/6
        (ecto) lib/ecto/repo/schema.ex:208: anonymous fn/14 in Ecto.Repo.Schema.do_insert/4
        (ecto) lib/ecto/repo/schema.ex:774: anonymous fn/3 in Ecto.Repo.Schema.wrap_in_transaction/6
        (ecto) lib/ecto/adapters/sql.ex:576: anonymous fn/3 in Ecto.Adapters.SQL.do_transaction/3
        (db_connection) lib/db_connection.ex:1283: DBConnection.transaction_run/4
        (db_connection) lib/db_connection.ex:1207: DBConnection.run_begin/3
        (db_connection) lib/db_connection.ex:798: DBConnection.transaction/3
        (ecto) lib/ecto/repo/schema.ex:125: Ecto.Repo.Schema.insert!/4
        priv/repo/seeds.exs:311: (file)
        (elixir) lib/code.ex:677: Code.require_file/2
To access a local copy of changelog via a custom host, e.g.
changelog which resolves to a host on your network, run e.g. `export
HOST=changelog` before starting the changelog app locally, via
`docker-compose up`. Requests to http://changelog will now be serviced
by the proxy container & requests to http://changelog:4000 will be
serviced by the app container.

[#158222089]
It feels more descriptive and easier to remember.

[#158222089]
We want to fail make contrib if exit code is not 2 (SIGINT - "Interrupt
from keyboard"), which is what happens on CTRL+C. This is the only non-0
exit code that is valid happy-path, otherwise we assume that
docker-compose up failed due to app dependencies.

The specific scenario which this fix mitigates against: if there is an
existing app image which has different dependencies baked in than what
is defined in the mix files, contrib will fail. The cached app image
will be re-created when build gets called since the layers have changed.

To always perform a clean container shutdown, we explicitly run
docker-compose down on SIGINT.

[#158222089]
@gerhard
Copy link
Member Author

gerhard commented Oct 9, 2018

@jerodsanto rebased against master, worked in the discussed build before contrib gotcha & captured gmake on macOS in README.

@codyjames codyjames removed their request for review October 9, 2018 16:05
@jerodsanto jerodsanto merged commit 86891f4 into master Oct 9, 2018
@gerhard gerhard deleted the docker-dev branch October 9, 2018 20:41
@gerhard gerhard changed the title 🛠️ I can develop the latest changelog.com version locally, with a single command: docker-compose up I can develop the latest changelog.com version locally, with a single command: docker-compose up Oct 9, 2018
gerhard added a commit that referenced this pull request Oct 13, 2018
This introduces a new make target, list-lp-secrets. Its purpose
is to list the majority of secrets stored in LastPass and used by this
app, as well as https://github.com/thechangelog/infrastructure, specifically
https://github.com/thechangelog/infrastructure/blob/master/.envrc

This commit removes .envrc, which is now redundant. All secret
environment variables are now captured in individual make targets.  These
targets are considered part of the private interface and are therefore
not exposed in `make help`.  For example, to load GitHub secrets in the
shell context, run `eval $(make github)`.  To load all secrets in the
shell context, run `eval $(make list-lp-secrets)`.

    Remember `gmake` vs `make` gotcha on OS X:
    86891f4

The only .envrc functionality that was lost is `PATH_add script`.  This
was basically adding $PWD/script to $PATH and enabling users to run any
script from the `script` dir by invoking it directly: e.g. `run` vs
`script/run`.  Since we've introduced a Docker-based workflow in #246,
these scripts are less useful and I imagine them disappearing completely
in the not too distant future.  We might want to discuss this
some more with @jerodsanto before I take this idea to completion.

This commit also removes .envrc.personal, which is also redundant.
@jerodsanto picked up on the disconnect of requiring 2 separate .envrcs
instantly, so this commit solves the conflict by removing them
altogether.  The reason for needing 2 separate .envrcs is user-specific
secrets.  For example, CIRCLE_TOKEN is a personal API token for CircleCI.
I have my own, Jerod has his own, etc.  We can't reference a single
LastPass entry, since that would point to a specific token, which would
be the wrong one for either myself or Jerod.  Yes, we could use $USER to
distinguish between users, but it adds complexity, leaks knowledge
across boundaries, feels too smart for the value that it adds.  The
alternative that was chosen is to not version control .envrc and instead
provide a template that anyone can use as a starting point for their own
individual .envrc.  I am always open to better alternatives.

[finishes #161188291]
gerhard added a commit that referenced this pull request Nov 2, 2018
This is something that the Concourse does via the `build-runtime` job.
Since we want to replace Concourse with CircleCI, we'll need to be able
to do this locally, with a single command.

The initial Dockerfile was stored in [infrastructure/roles/dotcom/files/docker/runtime/Dockerfile](https://github.com/thechangelog/infrastructure/blob/deef8db2f92becdcb796096d7077d683dd5ba3cb/roles/dotcom/files/docker/runtime/Dockerfile)
and in [changelog.com/Dockerfile](https://github.com/thechangelog/changelog.com/blob/0611abb43f83792b7ff9e60ded0094ec0d6d5395/Dockerfile) since [#246](#246).
As of this story, `changelog.com/Dockerfile` will be split into multiple files: `Dockerfile.runtime` & `Dockerfile` which will build on `thechangelog/runtime`, an artefact of `Dockerfile.runtime`.

`thechangelog/runtime` image is used by [CircleCI for running tests](https://circleci.com/gh/thechangelog/changelog.com/240#tests/containers/0) (see the **Spin up Environment** output),
and by docker-compose when [running the app locally](https://github.com/thechangelog/changelog.com/blob/aa95d83f4dec757b2a7e0cc086253e16065bf8db/Makefile#L88-L95).

By the way, this is the place where Elixir & Erlang used by
changelog.com are updated, as seen in #158032996, #150763329 & others.

[#161686902]
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

4 participants