docker compose overlay file for editable backend container #1098
Conversation
…t when running unit tests
…sed for dev says 8000
WalkthroughWalkthroughThe recent updates enhance the development environment and workflow significantly. Changes include the integration of user and group ID variables, introduction of container configurations for backend and frontend, and adaptation of Docker Compose commands to utilize these settings. Furthermore, new Makefile targets streamline development tasks, backend Docker and Compose files are refined for improved service setup, and backend configuration now includes hardcoded values for simplicity. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: .coderabbit.yaml
Files selected for processing (5)
- Makefile (1 hunks)
- spiffworkflow-backend/dev.Dockerfile (1 hunks)
- spiffworkflow-backend/dev.docker-compose.yml (1 hunks)
- spiffworkflow-backend/src/spiffworkflow_backend/config/unit_testing.py (1 hunks)
- spiffworkflow-frontend/dev.docker-compose.yml (1 hunks)
Additional comments: 2
spiffworkflow-backend/dev.Dockerfile (1)
- 14-14: Using
poetry install --no-rootis a good practice to avoid installing the project package itself. However, ensure that this aligns with the project's requirements, especially if the project package needs to be available for certain commands or scripts to run correctly.spiffworkflow-backend/src/spiffworkflow_backend/config/unit_testing.py (1)
- 8-8: Hardcoding the
SPIFFWORKFLOW_BACKEND_PERMISSIONS_FILE_NAMEto "unit_testing.yml" simplifies the configuration for testing. Ensure that this file is appropriately configured and available in the testing environment.
| @@ -3,6 +3,7 @@ services: | |||
| build: | |||
| context: spiffworkflow-frontend | |||
| dockerfile: dev.Dockerfile | |||
| user: "${RUN_AS}" | |||
There was a problem hiding this comment.
The addition of the user: "${RUN_AS}" configuration is a good practice for managing file permissions between the host and the container. However, it's crucial to ensure that the ${RUN_AS} environment variable is well-documented and validated to prevent potential runtime issues. Consider adding documentation on how to set this variable and validating its presence before container startup.
There was a problem hiding this comment.
@burnettk, I appreciate your perspective. The intention was to highlight the benefits of documenting and validating the ${RUN_AS} environment variable to ensure a seamless development experience, especially for new contributors or when changes are made to the development setup. It's indeed important to balance the guidance with the practicalities of the development process. Thank you for pointing that out.
| RUN apt-get update \ | ||
| && apt-get install -y -q \ | ||
| gcc libssl-dev libpq-dev default-libmysqlclient-dev \ | ||
| pkg-config libffi-dev git-core curl |
There was a problem hiding this comment.
Installing system dependencies with apt-get install is necessary, but consider cleaning up the apt cache with rm -rf /var/lib/apt/lists/* at the end of the command to reduce the image size.
| pkg-config libffi-dev git-core curl | ||
|
|
||
| RUN pip install --upgrade pip | ||
| RUN pip install poetry==1.6.1 pytest-xdist |
There was a problem hiding this comment.
Specifying the Poetry version is a good practice for ensuring consistent builds. However, consider using a lock file or a more controlled environment to manage the exact versions of all dependencies, including pytest-xdist, to further enhance reproducibility.
| # TODO: would like to figure out the permissions issue that is preventing this | ||
| # | ||
| #user: "${RUN_AS}" |
There was a problem hiding this comment.
The commented-out user configuration due to permissions issues is noteworthy. Resolving this to align with the frontend's approach of running containers with specific user permissions would enhance consistency and potentially avoid permission issues with mounted volumes. Consider investigating and documenting the permissions issue further.
| MY_USER := $(shell id -u) | ||
| MY_GROUP := $(shell id -g) | ||
| ME := $(MY_USER):$(MY_GROUP) |
There was a problem hiding this comment.
Defining MY_USER and MY_GROUP variables at the beginning of the Makefile is a good practice for managing file permissions consistently. Ensure that there's documentation on how these variables are used and any prerequisites for the development environment setup.
| DOCKER_COMPOSE ?= RUN_AS=$(ME) docker compose $(YML_FILES) | ||
| IN_BACKEND ?= $(DOCKER_COMPOSE) run $(BACKEND_CONTAINER) | ||
| IN_FRONTEND ?= $(DOCKER_COMPOSE) run $(FRONTEND_CONTAINER) |
There was a problem hiding this comment.
The use of RUN_AS=$(ME) in Docker Compose commands is a smart way to ensure consistent user permissions. However, ensure that there's clear documentation on how these commands work and any necessary environment setup, especially for new contributors.
|
|
||
| SPIFFWORKFLOW_BACKEND_URL = "http://localhost:7000" |
There was a problem hiding this comment.
Introducing a hardcoded SPIFFWORKFLOW_BACKEND_URL value can simplify testing by providing a consistent base URL. However, consider the implications for testing in environments where the backend URL might differ, and ensure there's a way to override this value if necessary.
There was a problem hiding this comment.
i can't think of a case where you would want it to differ. respecting environment variables in this file was one of things making the unit tests less portable to this new environment. using good known values seems safest.
There was a problem hiding this comment.
@burnettk: Your point about prioritizing portability and simplicity in the test environment setup is well taken. Hardcoding SPIFFWORKFLOW_BACKEND_URL does indeed streamline the process and ensures consistency across different testing scenarios, which aligns with the objectives of making the development workflow more efficient. It's clear that in the context of your project, the benefits of this approach outweigh the potential limitations. Should the need arise to accommodate different backend URLs in the future, adjustments could be made to introduce flexibility without significantly complicating the current setup. Your strategy seems well-considered for the project's current requirements.
|
|
||
| SPIFFWORKFLOW_BACKEND_URL = "http://localhost:7000" |
There was a problem hiding this comment.
i can't think of a case where you would want it to differ. respecting environment variables in this file was one of things making the unit tests less portable to this new environment. using good known values seems safest.
| @@ -3,6 +3,7 @@ services: | |||
| build: | |||
| context: spiffworkflow-frontend | |||
| dockerfile: dev.Dockerfile | |||
| user: "${RUN_AS}" | |||
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- Makefile (1 hunks)
Additional comments: 17
Makefile (17)
- 1-3: Variables
MY_USER,MY_GROUP, andMEare well-defined, leveraging shell commands to dynamically assign user and group IDs. This approach ensures that file permissions are handled correctly across different development environments.- 5-7: Defining variables for the backend container and its development overlay is a good practice. It enhances modularity and makes the Makefile more maintainable by allowing easy updates to container names and paths.
- 8-8: Variables for the frontend container and its development overlay are defined without explicit line number annotations. This is consistent with the backend definitions and follows the same best practices for modularity and maintainability.
- 11-13: The
DOCKER_COMPOSEandIN_BACKEND,IN_FRONTENDcommand variables are well-structured, incorporating the user and group ID variables for consistent permission management. This setup facilitates running Docker Compose commands with the correct user permissions, reducing potential issues related to file ownership.- 22-24: The
build-imagestarget uses theDOCKER_COMPOSEvariable to build Docker images. This is a straightforward and effective way to encapsulate the build process. Ensure that all necessary images are included in the Docker Compose configurations to avoid partial builds.- 25-26: The
dev-envtarget depends onbuild-images,fe-npm-i, andbe-recreate-db, which is a logical sequence for setting up the development environment. This target ensures that the environment is fully prepared before any development begins.- 28-29: The
start-devtarget correctly uses theDOCKER_COMPOSEvariable to start the development environment in detached mode. This allows developers to continue using their terminal while the services are running.- 32-32: The
stop-devtarget is essential for cleanly shutting down the development environment. Using theDOCKER_COMPOSEvariable ensures consistency with other Docker-related commands.- 34-35: The
be-recreate-dbtarget is crucial for database management in the backend. It uses theIN_BACKENDvariable to run a script inside the backend container, ensuring the command is executed in the correct context.- 37-38: The
be-shtarget provides a convenient way to access the backend container's shell. This is useful for debugging and manual intervention when necessary.- 40-41: The
be-teststarget is well-defined, using Poetry within the backend container to run tests. This encapsulation ensures that tests are run in an environment identical to the production setup.- 43-44: The
be-tests-partarget uses the-n autooption with pytest to run tests in parallel, which can significantly reduce test execution time. However, ensure that tests are designed to be run in parallel without causing interference or race conditions.- 47-47: The
fe-lint-fixtarget automates the process of fixing linting issues in the frontend codebase. This is a valuable tool for maintaining code quality and consistency.- 49-50: The
fe-npm-itarget ensures that all necessary npm packages are installed in the frontend container. This is crucial for a consistent development environment.- 53-53: The
fe-shtarget provides a convenient way to access the frontend container's shell, similar to thebe-shtarget. This facilitates debugging and manual tasks within the frontend environment.- 55-56: The
take-ownershiptarget is a practical solution for resolving file ownership issues that may arise from containerized development environments. It uses theMEvariable to ensure that the current user regains ownership of all project files.- 58-62: The
.PHONYdeclaration correctly lists all targets to ensure Make doesn't confuse them with filenames. This is a best practice for Makefiles to avoid unexpected behavior.
| YML_FILES := -f docker-compose.yml \ | ||
| -f $(BACKEND_DEV_OVERLAY) \ | ||
| -f $(FRONTEND_DEV_OVERLAY) |
There was a problem hiding this comment.
The YML_FILES variable concatenates the main Docker Compose file with the backend and frontend development overlays. This approach ensures flexibility and modularity in managing Docker Compose configurations. However, ensure that indentation is consistent across the Makefile to improve readability.
- -f $(BACKEND_DEV_OVERLAY) \
- -f $(FRONTEND_DEV_OVERLAY)
+ -f $(BACKEND_DEV_OVERLAY) \
+ -f $(FRONTEND_DEV_OVERLAY)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| YML_FILES := -f docker-compose.yml \ | |
| -f $(BACKEND_DEV_OVERLAY) \ | |
| -f $(FRONTEND_DEV_OVERLAY) | |
| YML_FILES := -f docker-compose.yml \ | |
| -f $(BACKEND_DEV_OVERLAY) \ | |
| -f $(FRONTEND_DEV_OVERLAY) |
| all: dev-env start-dev be-tests-par | ||
| @/bin/true |
There was a problem hiding this comment.
The all target is a convenient entry point for setting up the development environment. It's good practice to include a brief comment or documentation section explaining what this target does and when it should be used, to make it more accessible to new developers.
Would you like me to add a comment explaining the all target for clarity?
This provides an overlay file which when used along with the existing docker-compose.yml, allows for editing backend code. Many thanks to @burnettk for getting the unit tests to pass, that was driving me bonkers.
Summary by CodeRabbit