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

[Bug] Project does not support Docker with Pre-commit #184

Open
TimidRobot opened this issue Oct 7, 2021 · 9 comments
Open

[Bug] Project does not support Docker with Pre-commit #184

TimidRobot opened this issue Oct 7, 2021 · 9 comments
Assignees
Labels
💻 aspect: code Concerns the software code in the repository 🛠 goal: fix Bug fix 🟩 priority: low Low priority and doesn't need to be rushed 🚧 status: blocked Blocked & therefore, not ready for work

Comments

@TimidRobot
Copy link
Member

TimidRobot commented Oct 7, 2021

Description

The docker environment is the primary supported development environment. It offers better parity with production and testing than a purely local pipenv and related services. However, the pre-commit hooks (as currently documented in README.md) act against the local repository, not the docker environment.

Expectation

If pre-commit cannot be configured to work with the Docker environment, it should be dropped.

Regardless of wether pre-commit is dropped or its configuration is updated, the GitHub action should be updated to better minimize the delta between what is done in developer's environments and what is done via GitHub Actions.

Configurations

Additional context

@TimidRobot TimidRobot added good first issue New-contributor friendly help wanted Open to participation from the community 🟩 priority: low Low priority and doesn't need to be rushed 🏁 status: ready for work Ready for work 🛠 goal: fix Bug fix 💻 aspect: code Concerns the software code in the repository Hacktoberfest labels Oct 7, 2021
@ntachukwu
Copy link

I see this has the tag for "Hacktoberfest". I am an aspiring intern from Outreachy. Can I use this issue to better understand the code base so I can eventually make contributions on the "Licenses Machine Readable Information" project.

@TimidRobot
Copy link
Member Author

I see this has the tag for "Hacktoberfest". I am an aspiring intern from Outreachy. Can I use this issue to better understand the code base so I can eventually make contributions on the "Licenses Machine Readable Information" project.

@Th3nn3ss Yep! the "Hacktoberfest" tag is not meant to limit involvement.

@TimidRobot TimidRobot removed the good first issue New-contributor friendly label Sep 16, 2022
@Alig1493
Copy link

Alig1493 commented Oct 18, 2022

@TimidRobot I'm not sure I understand the issue I was able to use pre commit fine within the docker container. What behaviour are you aiming for exactly? I was also able to create a small service using the app service as a template and tried something like:
docker-compose run pre-commit git commit -a -m "some changes"
And it invoked pre-commit fine as well.

Screenshot

image

@TimidRobot
Copy link
Member Author

TimidRobot commented Oct 18, 2022

@Alig1493 Yes, you can manually invoke it on the docker container, but the primary purpose of pre-commit is to run prior to each commit (as a git hook). Also, you can run it on the existing app service (ex. docker compose exec app pre-commit run --show-diff-on-failure --color=always --all-files).

While it may be easy enough to add a docker command as a git pre-commit hook, I expect a decent developer experience will require a helper script so that appropriate error messages are issued if a commit is tried and the docker services are unavailable. Ideally, it would be nice to find someone who has written about their experience using docker with git hooks. I'm not convinced a good developer experience is possible.

@Tapo41

This comment was marked as outdated.

@vanamaabhi2004

This comment was marked as outdated.

@TimidRobot
Copy link
Member Author

Please see Contribution Guidelines — Creative Commons Open Source for how we manage issues and PRs (we generally don't assign issues prior to resolution).

@dishak
Copy link

dishak commented Jun 21, 2024

@TimidRobot ,
I have curated a plan of action for the issue. Can you please review it and let me know if I have understood the problem correctly and if any further improvements in the plan of action further. Once if the proposal is appealing I will go forward with the implementation. Here is the link of proposal : https://tan-shampoo-b5f.notion.site/Proposal-for-Using-pre-commit-in-Dockerized-Application-83f5e66ee42a46c994d71571ae7ad44d . Thank you !!

@TimidRobot
Copy link
Member Author

@dishak

I have curated a plan of action for the issue. Can you please review it and let me know if I have understood the problem correctly and if any further improvements in the plan of action further. Once if the proposal is appealing I will go forward with the implementation. Here is the link of proposal : https://tan-shampoo-b5f.notion.site/Proposal-for-Using-pre-commit-in-Dockerized-Application-83f5e66ee42a46c994d71571ae7ad44d . Thank you !!

Proposal for Using pre-commit in Dockerized Application

Problem Description:

Implementing pre-commit hooks effectively within a Dockerized application to ensure consistent code quality and adherence to standards during development.


Plan 1: Integration of pre-commit Inside Docker Container

Proposed Plan of Action:

*** Modify docker-compose.yml:

Update the docker-compose.yml file to include pre-commit as part of the service definition for seamless integration.

version: '3'
services:
  my-service:
    build: .
    volumes:
      - .:/app  # Mount current directory to /app inside the container
    command: pre-commit run --all-files

*** Initialize Git Repository:

Configure the my-service container to initialize a Git repository upon startup. Use appropriate bind mounts to persist Git data across container restarts.

This approach automates the application of pre-commit hooks without manual intervention. Developers simply run docker-compose up to start the application with pre-commit hooks applied.

My concerns with this approach:

  1. There's no need to set the command: to pre-commit or create an additional container. The existing app container could be configured to support pre-commit.
  2. To allow pre-commit to execute within the container via a git hook, any commits (git commit) would need to be executed as docker command (ex. docker compose exec app git commit).
  3. Additionally, git would have to be fully configured for the developer within the container, significantly increasing the complexity.

Plan 2: Leveraging Hot Reloading Tools and Running pre-commit Locally

Proposed Plan of Action:

*** Utilize Hot Reloading Tools:

Implement hot reloading tools and bind mounts within the app service to detect code changes dynamically. This avoids the need to restart containers frequently during development.

Example docker-compose.yml snippet:

version: '3'
services:
  my-service:
    build: .
    volumes:
      - .:/app  # Mount current directory to /app inside the container
    command: uvicorn app:app --reload

*** pre-commit will be executed locally

There are pros and cons which can be of course discussed but, please let me know if I have got hold of the problem and solution draft if is right approach to the problem.

My concerns with this approach:

  1. There's no need to create an additional container. The existing app container (which uses Django) already supports hot reloading.
  2. Pre-commit can be configured to execute locally via pipenv.

Thank you for writing up the proposal. Having additional methods documented helps me organize my own thinking. I think this issue, as it is currently written, makes too many assumptions and/or doesn't properly document the different contexts the app is used in.

@TimidRobot TimidRobot added 🚧 status: blocked Blocked & therefore, not ready for work and removed help wanted Open to participation from the community 🏁 status: ready for work Ready for work Hacktoberfest labels Jun 21, 2024
@TimidRobot TimidRobot self-assigned this Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💻 aspect: code Concerns the software code in the repository 🛠 goal: fix Bug fix 🟩 priority: low Low priority and doesn't need to be rushed 🚧 status: blocked Blocked & therefore, not ready for work
Projects
Status: Backlog
Development

No branches or pull requests

6 participants