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

Improve local development and startup via docker #38

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

Conversation

MrPandir
Copy link
Contributor

@MrPandir MrPandir commented Jun 15, 2024

Motivation

  • Ensuring an easier start for new contributors.
  • Avoiding the need to install necessary development tools on the local system.
  • Ensuring the project runs in the same environment, eliminating potential issues associated with the system environment.

Purposes

  • Add the ability to run a project locally in docker compose.
  • Add dev container support.
  • Document this in the README.

Known bugs

  • An error may occur when launching an app container if the database was not ready to accept requests.
    This often happens if the volume does not exist, in which case the container needs additional time to create the volume.
    Solution: add a health check for the database container.

  • Dev container fails at build.
    Cause: dev container tries to override the configuration and fails.

    Logs
     [2024-06-15T19:18:33.904Z] Docker Compose override file for building image:
     services:
       app:
         build:
           dockerfile: /var/folders/gz/srd0zmvj357gnb8_lk28mj400000gn/T/devcontainercli/container-features/0.64.0-1718479113903/Dockerfile-with-features
           args:
             - BUILDKIT_INLINE_CACHE=1
             - _DEV_CONTAINERS_BASE_IMAGE=dev_container_auto_added_stage_label
     
     [2024-06-15T19:18:33.924Z] Start: Run: docker-compose --project-name dudes -f /Users/user/Desktop/dudes/compose-dev.yaml -f /Users/user/Library/Application Support/Code - Insiders/User/globalStorage/ms-vscode-remote.remote-containers/data/docker-compose/docker-compose.devcontainer.build-1718479113904.yml -f /Users/user/Library/Application Support/Code - Insiders/User/globalStorage/ms-vscode-remote.remote-containers/data/docker-compose/docker-compose.devcontainer.containerFeatures-1718479113923-c8851d69-5ce9-45d0-b5cc-8dc9b122edb3.yml up -d --no-recreate
     [2024-06-15T19:18:33.973Z] service "app" declares mutualy exclusive dockerfile and dockerfile_inline: invalid compose project
    
  • When opening http://localhost:4300/client on the server, you can’t find the user.
    Not sure if this error is related to deployment. Maybe you need to run pnpm run db:generate.
    Update: This is not a deployment issue.

    Logs

    client_prisma_error.log

  • There is no mounting of the volume with project files in the app container.
    Currently, the project is copied during the creation of the image container.

Addition

At the moment, I can't ensure that the project works correctly in docker compose because the Twitch console refuses to register a new application due to the link not corresponding to the https protocol, even for links that have the https protocol.
It may be worth adding nginx to docker compose for development, which will allow testing some things locally.

@MrPandir
Copy link
Contributor Author

Development in Docker is already working. But this is not yet documented in the README.
And there is still the problem of creating the database for the first time. It is worth adding a database health check and only then launching the app service.

@MrPandir MrPandir changed the title Trying to improve local launch Improve local development and startup via docker Jun 26, 2024
@MrPandir
Copy link
Contributor Author

Basically this is the finished stage.
But I would like to add nginx. I was able to add nginx and run it, but it requires me to change the nginx/default.conf file as it has all ports set to 3000. 🤔

@MrPandir MrPandir marked this pull request as ready for review June 26, 2024 18:13
Copy link
Owner

@inferst inferst left a comment

Choose a reason for hiding this comment

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

I left some comments. Thank you for interest in the project. I appreciate it.

compose.dev.yaml Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

It would be better if we don't change current compose filenames. Also for local TS development it's not necessary to have the whole application inside a container. I suggest delete app part or create new compose file specifically for dev containers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be better if we don't change current compose filenames.

I think otherwise, it's worth changing for these reasons:

Also for local TS development it's not necessary to have the whole application inside a container. I suggest delete app part or create new compose file specifically for dev containers.

My interpretation of what you are suggesting:

  • .devcontainer/compose.app.yaml - сontains the project
  • compose.db.yaml - сontains only postgres

I also want to add nginx to the development environment so that I can test things like 7tv emotes that require a proxy.
It also brings you closer to the production environment, which helps identify potential problems early on.
This will add one more file:

  • compose.nginx.yaml - сontains only nginx

I can do that. Is that okay with you?

Copy link
Owner

Choose a reason for hiding this comment

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

I think otherwise, it's worth changing for these reasons:

Shorter name is easier to type
Follows modern Docker Compose naming conventions
Reduces potential for typos

It makes sense, but if we want to rename compose files we should also rename docker-compose.production.yml. I would like to avoid inconsistency. Either we rename all files or leave it as is and rename them in separate branch.

My interpretation of what you are suggesting:
.devcontainer/compose.app.yaml - сontains the project

Looks good.

compose.db.yaml - сontains only postgres

Let's leave dev or development. Potentially it can contain other services for dev environment.

I also want to add nginx to the development environment so that I can test things like 7tv emotes that require a proxy.

My suggestion is place nginx config for 7tv proxy to the dev compose file. But only proxy.

Copy link
Contributor Author

@MrPandir MrPandir Jun 27, 2024

Choose a reason for hiding this comment

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

It makes sense, but if we want to rename compose files we should also rename docker-compose.production.yml. I would like to avoid inconsistency.

I want to rename docker-compose.production.yaml to compose.prod.yaml, but this PR should not affect the production environment.

Either we rename all files or leave it as is and rename them in separate branch.

So yes, it's worth doing this in a separate PR in which to rename all the files.

Let's leave dev or development. Potentially it can contain other services for dev environment.
My suggestion is place nginx config for 7tv proxy to the dev compose file. But only proxy.

I thought about that, but would you be happy with posgres and nginx running in the same file?

Changed structure:

.
├── .devcontainer/
│   └── compose.app.yaml ─ project
├── compose.dev.yaml ─ posgres and nginx
└── compose.prod.yaml ─ no changes :D

Copy link
Owner

@inferst inferst Jun 27, 2024

Choose a reason for hiding this comment

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

Let's leave dev or development. Potentially it can contain other services for dev environment.
My suggestion is place nginx config for 7tv proxy to the dev compose file. But only proxy.

I thought about that, but would you be happy with posgres and nginx running in the same file?

If I could serve app locally with pnpm run dev I would be happy :D

So yes, it's ok

.gitignore Show resolved Hide resolved
@MrPandir
Copy link
Contributor Author

MrPandir commented Jun 27, 2024

  • Add some VS_Сode extensions to devcontainer.
  • Add pnpm run db:seed execution to devcontainer.
  • Move README style changes to a separate PR, leaving only those related to devcontainer and docker.
  • Add container with nginx.
  • Do not migrate when the container starts. Execute when creating a container.
  • Is it possible to add caching for devcontainer files? Because the startup is too long, by my standards.
  • Move app service to compose.dev.app.yaml.
  • Mount global git config in read-only mode.
  • Add a connection description for viewing the database.
  • Add information about adding extensions via defaultExtensions.

@MrPandir MrPandir force-pushed the main branch 3 times, most recently from df0e6e5 to 4c691dd Compare July 7, 2024 19:58
@MrPandir
Copy link
Contributor Author

Added nginx and website to the compose file. This now looks like the production version, except that it is running in dev mode.
nginx will also be able to proxy requests if the project is run locally, since it uses the host machine environment.

nginx works on port 8080, to make twitch application authorization work, it should be added to the list of allowed redirect urls.
Screenshot 2024-07-20 at 13 49 05
@inferst do it please.

@inferst
Copy link
Owner

inferst commented Jul 20, 2024

This PR is getting more complicated than I expected. It takes time to find out what happens.

Again, for me it's not convenient to have the whole app in dev container.

@MrPandir
Copy link
Contributor Author

Again, for me it's not convenient to have the whole app in dev container.

I'm still working on it. It's the next thing I intend to do.

I'm turning PR back into draft until it's in the final stages. As soon as it works as intended and satisfies everyone and is fully documented in the README, I will reopen it for merging.

@MrPandir MrPandir marked this pull request as draft July 20, 2024 14:09
@MrPandir MrPandir marked this pull request as ready for review July 26, 2024 20:02
@MrPandir MrPandir requested a review from inferst July 26, 2024 20:03
@MrPandir
Copy link
Contributor Author

MrPandir commented Oct 3, 2024

  • Replace docker compose run with docker compose up

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.

2 participants