tl-its-umich-edu / canvas-app-explorer

A Web application that presents a list of Canvas external (LTI) tools with details. When integrated within Canvas, the user can search for specific LTI tool(s), and add or remove those tools from Canvas courses.
Apache License 2.0
4 stars 6 forks source link

Make sure `package*.json` are available during build (#42) #43

Closed ssciolla closed 3 years ago

ssciolla commented 3 years ago

This PR modifies the Dockerfile to ensure that the package*.json files are available during the build process (for npm install). In addition, it modifies the volume mapping in docker-compose.yml to correctly (?) prevent node_modules in the local repository from being copied into the container (see https://stackoverflow.com/questions/29181032/add-a-volume-to-docker-but-exclude-a-sub-folder; you may also be able to use .dockerignore, ignoring node_modules). The PR aims to resolve #42.

ssciolla commented 3 years ago

@danie1zhang, not a big deal, but couple comments: 1) Try to give explicit approvals to others' PRs. It can be as simple as providing a review, writing "LGTM", and clicking "Approve". Sometimes we'll require that PRs be approved before they can be merged. See https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/about-pull-request-reviews 2) Matt may have a preference on a merge strategy, so I'd discuss that with him going forward (e.g. "Squash and merge", "Rebase and merge", or "Create a merge commit").

danie1zhang commented 3 years ago

@ssciolla Thanks for the advice and help, I really appreciate it. I'll be sure to discuss with Matt about merging going forward.