Tide DB #22
Tide DB #22
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
Summary by CodeRabbit
WalkthroughThe root Makefile has been refactored to delegate all detailed logic and targets to modular Makefile snippets under Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Makefile
participant env.mk
participant app.mk
participant db.mk
participant build.mk
participant logs.mk
Developer->>Makefile: make <target>
alt Environment targets
Makefile->>env.mk: include and dispatch env:init/env:check/env:fresh/env:print
else Application targets
Makefile->>app.mk: include and dispatch format/fresh/audit/watch
else Database targets
Makefile->>db.mk: include and dispatch db:local/db:seed/db:up/db:migrate/etc.
else Build targets
Makefile->>build.mk: include and dispatch build:app/build:run/build:release/etc.
else Log targets
Makefile->>logs.mk: include and dispatch logs:fresh/logs:bin:fresh
end
sequenceDiagram
participant Developer
participant db.mk
participant Docker
participant PostgreSQL
participant migrate/migrate
Developer->>db.mk: make db:migrate
db.mk->>Docker: Run migrate/migrate container
Docker->>migrate/migrate: Mount migration files, connect to DB
migrate/migrate->>PostgreSQL: Execute migration commands
PostgreSQL-->>migrate/migrate: Migration result
migrate/migrate-->>Developer: Output migration status
sequenceDiagram
participant Developer
participant build.mk
participant logs.mk
participant GoCompiler
Developer->>build.mk: make build:app
build.mk->>logs.mk: logs:bin:fresh (reset logs)
build.mk->>GoCompiler: go build with version and tags
GoCompiler-->>build.mk: Build artefact
build.mk-->>Developer: Built binary in bin directory
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 20
🛑 Comments failed to post (20)
.env (3)
1-5: 🧹 Nitpick (assertive)
Consolidate redundant environment variables: The variables
ENV_APP_ENV_TYPEandENV_APP_ENVappear to serve the same purpose. Consider merging into a singleENV_APP_ENVto reduce confusion.
26-28:
⚠️ Potential issueTypo in environment variable name and spelling:
EN_DB_BIN_DIRis missing the leadingVand the comment has spelling errors. This will break references to the binary directory.-EN_DB_BIN_DIR="" # Local posgreSQL instalation directory. Example: /Library/PostgreSQL/17/bin/ +ENV_DB_BIN_DIR="" # Local PostgreSQL installation directory. Example: /Library/PostgreSQL/17/bin/📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.ENV_DB_TIMEZONE="Asia/Singapore" ENV_DB_BIN_DIR="" # Local PostgreSQL installation directory. Example: /Library/PostgreSQL/17/bin/ ENV_DB_URL="" # Example: postgresql://gocanto-user:gocanto-password@postgres:5432/gocanto-db?sslmode=require
19-23: 🧹 Nitpick (assertive)
Consider quoting values with special characters: If any of these variables (e.g.
ENV_DB_USER_PASSWORD) may contain special characters or spaces, wrapping them in quotes will ensure correct parsing:-ENV_DB_USER_PASSWORD=gocanto-password +ENV_DB_USER_PASSWORD="gocanto-password"📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.# --- App db secrets ENV_DB_USER_NAME=gocanto-user ENV_DB_USER_PASSWORD="gocanto-password" ENV_DB_DATABASE_NAME=gocanto-db ENV_DB_PORT=5432docker-compose.yml (1)
12-12:
⚠️ Potential issueFix YAML indentation for
env_file: The list item underenv_fileis over‑indented, leading to a YAML parse error. It should be two spaces deeper thanenv_file::- - .env.old + - .env.old📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.- .env.old🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 12-12: wrong indentation: expected 6 but found 8
(indentation)
config/makefile/consts.mk (1)
10-10:
⚠️ Potential issueFix Makefile conditional assignment syntax: The line uses
? =instead of?=and will not be parsed correctly. Update to:-ROOT_EXAMPLE_ENV_FILE? = $(ROOT_PATH)/.env.example +ROOT_EXAMPLE_ENV_FILE ?= $(ROOT_PATH)/.env.exampleconfig/makefile/logs.mk (2)
1-3: 🧹 Nitpick (assertive)
Enhance robustness by quoting path: Wrapping the directory in quotes handles any spaces in
$(STORAGE_PATH):-find $(STORAGE_PATH)/logs -maxdepth 1 -type f -not -name ".gitkeep" -delete +find "$(STORAGE_PATH)/logs" -maxdepth 1 -type f -not -name ".gitkeep" -delete📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.logs\:fresh: find "$(STORAGE_PATH)/logs" -maxdepth 1 -type f -not -name ".gitkeep" -delete
4-7: 🧹 Nitpick (assertive)
Ensure consistency with quoting: In
logs:bin:fresh, quote$(BIN_LOGS_PATH)for reliability:-@mkdir -m 777 $(BIN_LOGS_PATH) -@touch $(BIN_LOGS_PATH)/.gitkeep +@mkdir -m 777 "$(BIN_LOGS_PATH)" +@touch "$(BIN_LOGS_PATH)/.gitkeep"📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.logs\:bin\:fresh: @rm -rf "$(BIN_LOGS_PATH)" @mkdir -m 777 "$(BIN_LOGS_PATH)" @touch "$(BIN_LOGS_PATH)/.gitkeep"config/makefile/env.mk (2)
3-3: 🧹 Nitpick (assertive)
Consider removing the commented debug code
This commented line appears to be debug code that was left in the final version. Consider removing it to keep the Makefile clean.
- #echo "---> $(ROOT_ENV_FILE) <> $(ROOT_EXAMPLE_ENV_FILE) <----- | "📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
7-7: 🧹 Nitpick (assertive)
Use variable for consistency
You're hardcoding
.envhere while using$(ROOT_ENV_FILE)in theenv:inittarget. For consistency, consider using the variable here as well.- @if [ ! -f .env ]; then \ + @if [ ! -f $(ROOT_ENV_FILE) ]; then \config/makefile/build.mk (3)
10-13: 🧹 Nitpick (assertive)
Inconsistency in directory handling between build targets
The
build:app:linuxtarget changes directory before building, whilebuild:appdoesn't. This could lead to different build contexts and unexpected behaviour.For consistency, either add a
cd $(APP_PATH) &&to thebuild:apptarget or remove it frombuild:app:linux.
5-5: 💡 Verification agent
🧩 Analysis chain
Verify existence of .env.production
The build process copies
.env.productionto the binary directory, but doesn't verify that this file exists. Consider adding a check to prevent build failures if this file is missing.
🏁 Script executed:
#!/bin/bash # Check if .env.production exists in the expected location if [ -f "$(pwd)/.env.production" ]; then echo "Found .env.production file" else echo "WARNING: .env.production file not found at $(pwd)/.env.production" echo "This might cause the build:app target to fail" fiLength of output: 327
Add an existence check for
.env.productionbefore copying
Without this guard,cp $(ROOT_PATH)/.env.production $(BIN_PATH)/.envwill fail with a confusing error if the file is missing. Please updateconfig/makefile/build.mk(around line 5) to include something like:# Ensure .env.production exists test -f $(ROOT_PATH)/.env.production || \ { echo >&2 "Error: .env.production not found at $(ROOT_PATH)/.env.production"; exit 1; } cp $(ROOT_PATH)/.env.production $(BIN_PATH)/.env && \ …rest of your commands…This will provide a clear, early error message and prevent downstream build confusion.
18-20: 🧹 Nitpick (assertive)
Add validation for git tag existence
The
build:releasetarget doesn't check if the tag already exists before attempting to create and push it. This could lead to errors if the tag already exists.build\:release: - git tag v$(V) - @read -p "Press enter to confirm and push to origin ..." && git push origin v$(V) + @if git rev-parse v$(V) >/dev/null 2>&1; then \ + echo "Tag v$(V) already exists. Please use a different version."; \ + exit 1; \ + else \ + git tag v$(V); \ + read -p "Press enter to confirm and push to origin ..." && git push origin v$(V); \ + fiMakefile (2)
32-32: 🧹 Nitpick (assertive)
Variable assignment inconsistency
Most variables use
?=which allows overriding from the command line, butROOT_EXAMPLE_ENV_FILEuses:=which prevents overriding. Consider using?=for consistency unless there's a specific reason to prevent overriding.-ROOT_EXAMPLE_ENV_FILE := $(ROOT_PATH)/.env.example +ROOT_EXAMPLE_ENV_FILE ?= $(ROOT_PATH)/.env.example
40-44: 🧹 Nitpick (assertive)
Consider adding comments for included Makefiles
To improve maintainability, consider adding brief comments explaining the purpose of each included Makefile. This would help newcomers understand the structure without having to examine each file.
-include ./config/makefile/env.mk -include ./config/makefile/app.mk -include ./config/makefile/db.mk -include ./config/makefile/build.mk -include ./config/makefile/logs.mk +# Environment setup and validation +include ./config/makefile/env.mk +# Application management commands +include ./config/makefile/app.mk +# Database operations +include ./config/makefile/db.mk +# Build and release processes +include ./config/makefile/build.mk +# Log management +include ./config/makefile/logs.mk📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.# Environment setup and validation include ./config/makefile/env.mk # Application management commands include ./config/makefile/app.mk # Database operations include ./config/makefile/db.mk # Build and release processes include ./config/makefile/build.mk # Log management include ./config/makefile/logs.mkconfig/makefile/app.mk (2)
20-23: 🧹 Nitpick (assertive)
Add verification for air installation
The
watchtarget assumesairis installed but doesn't check for its presence. Consider adding a check to provide a helpful error message if it's missing.watch: # --- Works with (air). # https://github.com/air-verse/air - cd $(APP_PATH) && air + @if ! command -v air >/dev/null 2>&1; then \ + echo "Error: 'air' is not installed. Please install it from https://github.com/air-verse/air"; \ + exit 1; \ + fi + cd $(APP_PATH) && air
4-11: 🧹 Nitpick (assertive)
Add confirmation prompt for destructive operations
The
freshtarget performs several destructive operations without confirmation. Consider adding a prompt to prevent accidental data loss.fresh: - rm -rf $(DB_INFRA_DATA_PATH) && \ - docker compose down --remove-orphans && \ - docker container prune -f && \ - docker image prune -f && \ - docker volume prune -f && \ - docker network prune -f && \ - docker ps + @echo "Warning: This will remove all database data and prune Docker resources." + @read -p "Are you sure you want to continue? [y/N] " confirm; \ + if [[ $$confirm =~ ^[Yy] ]]; then \ + rm -rf $(DB_INFRA_DATA_PATH) && \ + docker compose down --remove-orphans && \ + docker container prune -f && \ + docker image prune -f && \ + docker volume prune -f && \ + docker network prune -f && \ + docker ps; \ + else \ + echo "Operation aborted"; \ + fidatabase/infra/migrations/000001_schema.up.sql (1)
155-170: 🧹 Nitpick (assertive)
Well-structured newsletters table, but consider subscription status management.
The newsletters table is generally well-designed with appropriate fields and constraints. The composite unique constraint on (email, name) makes sense if you're allowing the same email to subscribe to different newsletter types.
A few observations:
- Having both a unique constraint on email (line 158) and a composite unique constraint (line 165) might be redundant if each email can only subscribe once
- The separate timestamps for subscribed_at and unsubscribed_at effectively track subscription status
- The indexes on these timestamps will help with efficient querying of active subscribers
Consider adding a CHECK constraint to enforce business rules if a subscriber cannot be both subscribed and unsubscribed simultaneously.
You might consider adding a CHECK constraint to enforce that a newsletter entry can't have both subscribed_at and unsubscribed_at set at the same time:
CREATE TABLE newsletters ( id BIGSERIAL PRIMARY KEY, email VARCHAR(255) UNIQUE NOT NULL, name VARCHAR(255) NOT NULL, subscribed_at TIMESTAMP DEFAULT NULL, unsubscribed_at TIMESTAMP DEFAULT NULL, created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, UNIQUE (email, name) + CONSTRAINT chk_subscription_status CHECK ( + (subscribed_at IS NULL AND unsubscribed_at IS NOT NULL) OR + (subscribed_at IS NOT NULL AND unsubscribed_at IS NULL) OR + (subscribed_at IS NULL AND unsubscribed_at IS NULL) + ) );📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.--------------------------------------------------- NEWSLETTERS --------------------------------------------------------- CREATE TABLE newsletters ( id BIGSERIAL PRIMARY KEY, email VARCHAR(255) UNIQUE NOT NULL, name VARCHAR(255) NOT NULL, subscribed_at TIMESTAMP DEFAULT NULL, unsubscribed_at TIMESTAMP DEFAULT NULL, created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, UNIQUE (email, name) CONSTRAINT chk_subscription_status CHECK ( (subscribed_at IS NULL AND unsubscribed_at IS NOT NULL) OR (subscribed_at IS NOT NULL AND unsubscribed_at IS NULL) OR (subscribed_at IS NULL AND unsubscribed_at IS NULL) ) ); CREATE INDEX idx_newsletters_created_at ON newsletters(created_at); CREATE INDEX idx_newsletters_subscribed_at ON newsletters(subscribed_at); CREATE INDEX idx_newsletters_unsubscribed_at ON newsletters(unsubscribed_at);config/makefile/db.mk (3)
77-79:
⚠️ Potential issueCheck syntax of db:migrate:force command.
The command on line 79 appears to have a redundant "migrate" keyword which might cause syntax errors:
docker run -v $(DB_MIGRATE_VOL_MAP) --network ${ROOT_NETWORK} migrate/migrate migrate -path $(DB_MIGRATE_PATH) -database $(ENV_DB_URL) force $(version)The correct format based on the comment on line 78 and the migrate tool's documentation should be:
db\:migrate\:force: #migrate -path PATH_TO_YOUR_MIGRATIONS -database YOUR_DATABASE_URL force VERSION - docker run -v $(DB_MIGRATE_VOL_MAP) --network ${ROOT_NETWORK} migrate/migrate migrate -path $(DB_MIGRATE_PATH) -database $(ENV_DB_URL) force $(version) + docker run -v $(DB_MIGRATE_VOL_MAP) --network ${ROOT_NETWORK} migrate/migrate -path $(DB_MIGRATE_PATH) -database $(ENV_DB_URL) force $(version)
74-76: 🧹 Nitpick (assertive)
Consider adding filename validation for migration creation.
The target db:migrate:create creates new migration files but doesn't validate the input parameter
$(name). Consider adding a simple validation to ensure the filename follows a consistent pattern.db\:migrate\:create: + @if [ -z "$(name)" ]; then \ + echo "\n${RED}${PADDING}Error: Migration name is required. Example: make db:migrate:create name=add_users_table\n${NC}"; \ + exit 1; \ + fi docker run -v $(DB_MIGRATE_VOL_MAP) --network ${ROOT_NETWORK} migrate/migrate create -ext sql -dir $(DB_MIGRATE_PATH) -seq $(name)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.db\:migrate\:create: @if [ -z "$(name)" ]; then \ echo "\n${RED}${PADDING}Error: Migration name is required. Example: make db:migrate:create name=add_users_table\n${NC}"; \ exit 1; \ fi docker run -v $(DB_MIGRATE_VOL_MAP) --network ${ROOT_NETWORK} migrate/migrate create -ext sql -dir $(DB_MIGRATE_PATH) -seq $(name)
49-56:
⚠️ Potential issueVerify target references in db:secure.
There appear to be references to targets that don't match the defined ones:
- Line 50 calls
make freshbut there's no target named "fresh" in this file- Line 55 calls
make db:secure:permissionsbut the actual target is named "db:chmod" (line 57)Apply this correction:
db\:secure: - make fresh && \ + make db:fresh && \ rm -rf $(DB_INFRA_SERVER_CRT) && rm -rf $(DB_INFRA_SERVER_CSR) && rm -rf $(DB_INFRA_SERVER_KEY) && \ openssl genpkey -algorithm RSA -out $(DB_INFRA_SERVER_KEY) && \ openssl req -new -key $(DB_INFRA_SERVER_KEY) -out $(DB_INFRA_SERVER_CSR) && \ openssl x509 -req -days 365 -in $(DB_INFRA_SERVER_CSR) -signkey $(DB_INFRA_SERVER_KEY) -out $(DB_INFRA_SERVER_CRT) && \ - make db:secure:permissions + make db:chmod
There was a problem hiding this comment.
Actionable comments posted: 15
🔭 Outside diff range comments (5)
config/makefile/consts.mk (1)
32-32: 🧹 Nitpick (assertive)Add newline at end of file
Missing newline at the end of the file could cause issues with some tools.
DB_MIGRATE_VOL_MAP ?= $(DB_MIGRATE_PATH):$(DB_MIGRATE_PATH) +config/makefile/logs.mk (1)
8-8: 🧹 Nitpick (assertive)Add newline at end of file
Missing newline at the end of the file could cause issues with some tools.
@touch $(BIN_LOGS_PATH)/.gitkeep +config/makefile/env.mk (2)
13-13: 🧹 Nitpick (assertive)Add newline at end of file
Missing newline at the end of the file could cause issues with some tools.
+
1-13: 💡 Verification agent❓ Verification inconclusive
Define or import colour variables
The target uses
$(RED),$(NC), and$(BLUE)variables which aren't defined in this file. Ensure these are defined in a file included before this one or define them at the top of this file.
🏁 Script executed:
#!/bin/bash # Check if color variables are defined in included makefiles grep -r "RED.*=" config/makefile/ grep -r "BLUE.*=" config/makefile/ grep -r "NC.*=" config/makefile/Length of output: 107
To ensure we catch definitions anywhere in the repo, let’s search all
*.mkfiles for these colour variables:
🏁 Script executed:
#!/bin/bash # Search entire repository for colour variable definitions in Makefiles grep -R --include='*.mk' -n 'RED *[:+]?=' . grep -R --include='*.mk' -n 'BLUE *[:+]?=' . grep -R --include='*.mk' -n 'NC *[:+]?=' .Length of output: 137
Define missing ANSI colour variables
The
env:initandenv:checktargets use$(RED),$(BLUE)and$(NC), but no definitions were found in any*.mkacross the repo. You must either import a colour‑definitions file or declare them at the top ofconfig/makefile/env.mk.Suggested fix in
config/makefile/env.mk:+# ANSI colour definitions +RED := \033[0;31m +BLUE := \033[0;34m +NC := \033[0m + env\:init: #echo "---> $(ROOT_ENV_FILE) <> $(ROOT_EXAMPLE_ENV_FILE) <----- | " rm -f $(ROOT_ENV_FILE) && cp $(ROOT_EXAMPLE_ENV_FILE) $(ROOT_ENV_FILE)Or, if you have a shared
colors.mk, add at the top:include config/makefile/colors.mkconfig/makefile/build.mk (1)
21-21: 🧹 Nitpick (assertive)Add newline at end of file
Missing newline at the end of the file could cause issues with some tools.
@read -p "Press enter to confirm and push to origin ..." && git push origin v$(V) +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
Makefile(1 hunks)config/makefile/app.mk(1 hunks)config/makefile/build.mk(1 hunks)config/makefile/consts.mk(1 hunks)config/makefile/db.mk(1 hunks)config/makefile/env.mk(1 hunks)config/makefile/logs.mk(1 hunks)
🔇 Additional comments (8)
config/makefile/build.mk (1)
5-5: Consider security implications of copying production env fileCopying the production
.envfile to the bin directory might expose sensitive information if the bin directory is shared or backed up. Consider using a restricted production env file with only necessary variables.Makefile (2)
1-1: Setting bash as the shell is a good practice.This ensures consistent shell behaviour across different systems and is crucial for reliable Makefile execution, especially when using bash-specific features.
10-20: Well-organized .PHONY declarations.The separation of .PHONY targets by category (environment, build, database, logs) makes it clear which targets are available and how they're organized, improving maintainability.
config/makefile/app.mk (3)
1-2: Simple and useful format target.The use of
gofmtwith-wand-sflags ensures consistent code formatting across the project. The-sflag for simplifying code is particularly good practice.
13-19: Well-structured audit target for dependency management.The audit target provides a comprehensive view of dependencies across different parts of the project. This is valuable for maintaining clean dependency management.
26-28:✅ Verification successful
Well-defined macro for identifying external dependencies.
The external_deps macro effectively filters out internal dependencies, making it easier to track third-party dependencies. Ensure that the REPO_OWNER variable is properly defined elsewhere in the Makefile structure.
🏁 Script executed:
#!/bin/bash # Check if REPO_OWNER is defined in the Makefile structure grep -r "REPO_OWNER" --include="*.mk" config/makefile/Length of output: 315
REPO_OWNER is correctly defined
TheREPO_OWNERvariable is set inconfig/makefile/consts.mkwith a sensible default (the parent directory’s basename), so no further changes are needed.config/makefile/db.mk (2)
1-19: Well-structured variable definitions.The variable definitions are clearly organized by category (Docker, Paths, SSL, Migrations), which improves readability and maintainability.
64-68: Good use of Docker for migrations.Using Docker to run migrations ensures consistent behavior across environments. The use of volume mapping and network configuration ensures that migrations can access the necessary files and database.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
Makefile (1)
32-36: Add descriptive comments for included makefiles.Consider adding brief comments about what each included makefile is responsible for to help new developers understand the structure.
include ./config/makefile/env.mk include ./config/makefile/app.mk include ./config/makefile/db.mk include ./config/makefile/build.mk include ./config/makefile/logs.mk + +# env.mk - Environment setup and verification +# app.mk - Application formatting, watching, and dependency auditing +# db.mk - Database operations, migrations, and container management +# build.mk - Application build and run targets +# logs.mk - Log cleanup and managementconfig/makefile/db.mk (3)
83-85: Clarify documentation for thedb:migrate:forcetarget.The commented line appears to be a general example rather than the actual command being run. Consider updating the comment to make it clearer, especially since the actual command uses a slightly different syntax.
db\:migrate\:force: - #migrate -path PATH_TO_YOUR_MIGRATIONS -database YOUR_DATABASE_URL force VERSION + # Force migration version to specified value + # Usage: make db:migrate:force version=X docker run -v $(DB_MIGRATE_VOL_MAP) --network ${ROOT_NETWORK} migrate/migrate migrate -path $(DB_MIGRATE_PATH) -database $(ENV_DB_URL) force $(version)
53-60: 🛠️ Refactor suggestionRefactor
db:secureto avoid usingmake fresh.The
db:securetarget callsmake fresh, which performs aggressive cleanup operations that might have unintended side effects for database security setup. Consider scoping the cleanup to only what's needed.db\:secure: - make fresh && \ + docker compose down $(DB_DOCKER_SERVICE_NAME) --remove-orphans && \ rm -rf $(DB_INFRA_SERVER_CRT) && rm -rf $(DB_INFRA_SERVER_CSR) && rm -rf $(DB_INFRA_SERVER_KEY) && \ openssl genpkey -algorithm RSA -out $(DB_INFRA_SERVER_KEY) && \ openssl req -new -key $(DB_INFRA_SERVER_KEY) -out $(DB_INFRA_SERVER_CSR) && \ openssl x509 -req -days 365 -in $(DB_INFRA_SERVER_CSR) -signkey $(DB_INFRA_SERVER_KEY) -out $(DB_INFRA_SERVER_CRT) && \ - make db:secure:permissions + make db:chmod
59-62:⚠️ Potential issueFix inconsistency between target names.
The
db:securetarget callsmake db:secure:permissionsbut this target doesn't exist. Instead, adb:chmodtarget is defined. Either rename the target or update the call.db\:secure: make fresh && \ rm -rf $(DB_INFRA_SERVER_CRT) && rm -rf $(DB_INFRA_SERVER_CSR) && rm -rf $(DB_INFRA_SERVER_KEY) && \ openssl genpkey -algorithm RSA -out $(DB_INFRA_SERVER_KEY) && \ openssl req -new -key $(DB_INFRA_SERVER_KEY) -out $(DB_INFRA_SERVER_CSR) && \ openssl x509 -req -days 365 -in $(DB_INFRA_SERVER_CSR) -signkey $(DB_INFRA_SERVER_KEY) -out $(DB_INFRA_SERVER_CRT) && \ - make db:secure:permissions + make db:chmod -db\:chmod: +db\:secure\:permissions: chmod 600 $(DB_INFRA_SERVER_KEY) && chmod 600 $(DB_INFRA_SERVER_CRT)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
Makefile(1 hunks)config/makefile/db.mk(1 hunks)config/makefile/env.mk(1 hunks)
🔇 Additional comments (1)
config/makefile/env.mk (1)
16-21: Use consistent variable references for.envfile.The target checks for the
.envfile using the variable$(___ENV___FILE_NAME), which is good for consistency and maintainability. This properly addresses a previous review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
Makefile (1)
46-91: 🧹 Nitpick (assertive)🛠️ Refactor suggestion
Help target is clear; mark it phony and set as default
Thehelprecipe is well-structured and presents all targets neatly. To follow Make best practices:
- Declare
helpas a phony target to avoid conflicts with anyhelpfile:.PHONY: help- Optionally set it as the default goal so running
makewith no arguments prints help:.DEFAULT_GOAL := helpThis ensures
make helpis always invoked and makes the help page the first entry point.
♻️ Duplicate comments (1)
Makefile (1)
36-41: 🧹 Nitpick (assertive)Add inline comments for each included makefile
Great use of modular includes! To help new contributors quickly understand the purpose of each file, consider adding brief comments above each include, for example:# env.mk – environment setup and verification # db.mk – database container, migrations & SSL # app.mk – Go formatting, auditing & live reload # logs.mk – log directory cleanup # build.mk – build, run & release tasks
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Makefile(1 hunks)
🔇 Additional comments (1)
Makefile (1)
10-18: ANSI escape codes are well-defined and used consistently
All the colour constants (NC,BOLD,CYAN, etc.) are correctly declared and tied into thehelpoutput. Nice work maintaining readability in the terminal.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (8)
config/makefile/app.mk (2)
6-13: Consider scoping the 'fresh' target to project resources only.The
freshtarget aggressively removes Docker resources which might impact other Docker projects running on the same system. Consider limiting the cleanup to resources specific to this project.fresh: - rm -rf $(DB_INFRA_DATA_PATH) && \ - docker compose down --remove-orphans && \ - docker container prune -f && \ - docker image prune -f && \ - docker volume prune -f && \ - docker network prune -f && \ + rm -rf $(DB_INFRA_DATA_PATH) && \ + docker compose down --remove-orphans && \ + docker container prune -f --filter "label=project=$(ROOT_NETWORK)" && \ + docker image prune -f --filter "label=project=$(ROOT_NETWORK)" && \ + docker volume prune -f --filter "label=project=$(ROOT_NETWORK)" && \ + docker network prune -f --filter "name=$(ROOT_NETWORK)" && \ docker ps
22-25: Consider adding installation instructions for 'air'.While the comments mention what 'air' is used for, adding instructions on how to install it would help new developers get started more quickly.
watch: # --- Works with (air). # https://github.com/air-verse/air + # Install with: go install github.com/air-verse/air@latest cd $(APP_PATH) && airconfig/makefile/db.mk (3)
54-61: Missingdb:secure:permissionstarget & overly broad cleanup in db:secureI ran a search across all Makefiles and confirmed there is no definition for
db:secure:permissions(and no existingdb:chmodrule), so invoking it will fail. Also, callingmake freshhere likely does far more than just reset certificates.Please address both points:
• Define a thin permissions target (or invoke the correct existing rule). For example, in config/makefile/db.mk you might add:
db:secure:permissions: chmod 600 $(DB_INFRA_SERVER_KEY) $(DB_INFRA_SERVER_CSR) $(DB_INFRA_SERVER_CRT) chown root:root $(DB_INFRA_SERVER_KEY)• Scope your cleanup instead of
make fresh. If you only need to tear down the DB service, consider something like:docker compose down $(DB_DOCKER_SERVICE_NAME) --remove-orphans || trueThen your
db:securetarget could look roughly like:db:secure: docker compose down $(DB_DOCKER_SERVICE_NAME) --remove-orphans && \ rm -rf $(DB_INFRA_SERVER_CRT) $(DB_INFRA_SERVER_CSR) $(DB_INFRA_SERVER_KEY) && \ openssl genpkey -algorithm RSA -out $(DB_INFRA_SERVER_KEY) && \ openssl req -new -key $(DB_INFRA_SERVER_KEY) -out $(DB_INFRA_SERVER_CSR) && \ openssl x509 -req -days 365 \ -in $(DB_INFRA_SERVER_CSR) \ -signkey $(DB_INFRA_SERVER_KEY) \ -out $(DB_INFRA_SERVER_CRT) && \ make db:secure:permissions
62-63: Renamedb:chmodtodb:secure:permissions.The
db:chmodtarget seems to be what's expected bydb:secureasdb:secure:permissions. Consider renaming this target for consistency.-db\:chmod: +db\:secure\:permissions: chmod 600 $(DB_INFRA_SERVER_KEY) && chmod 600 $(DB_INFRA_SERVER_CRT)
82-83: Clarify documentation for thedb:migrate:forcetarget.The command could benefit from a brief comment explaining what it does and how to use it.
db\:migrate\:force: + # Force migration version to specified value + # Usage: make db:migrate:force version=X docker run -v $(DB_MIGRATE_VOL_MAP) --network $(ROOT_NETWORK) migrate/migrate migrate -path $(DB_MIGRATE_PATH) -database $(ENV_DB_URL) force $(version)Makefile (3)
5-5: Remove unusedRANDvariable
TheRANDvariable isn’t referenced in any of the included modules and appears to be dead code. Removing it will reduce clutter:-RAND := $(shell echo $$RANDOM)
30-30: FixREPO_OWNERcomputation
cd .. && basename "$$(pwd)"returns the parent directory’s name (often your home folder), not the repository owner. Consider extracting it from the Git remote URL:-REPO_OWNER := $(shell cd .. && basename "$$(pwd)") +REPO_OWNER := $(shell git remote get-url origin | sed -E 's#.*[:/](.*)/.*#\1#')
36-42: Document included Makefile snippets
Great modularisation! Please add brief comments above eachincludeto explain whathelpers.mk,env.mk,db.mk,app.mk,logs.mk, andbuild.mkprovide. For example:+# helpers.mk - common utility functions (dependency checks, padding, etc.) include ./config/makefile/helpers.mk +# env.mk - environment file init, validation & printing include ./config/makefile/env.mk +# db.mk - PostgreSQL container management, migrations & SSL setup include ./config/makefile/db.mk +# app.mk - code formatting, dependency audit & live‑reload targets include ./config/makefile/app.mk +# logs.mk - log rotation, cleanup & directory reset include ./config/makefile/logs.mk +# build.mk - Go build, run & release tasks include ./config/makefile/build.mk
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
Makefile(1 hunks)config/makefile/app.mk(1 hunks)config/makefile/db.mk(1 hunks)config/makefile/helpers.mk(1 hunks)database/infra/migrations/000001_schema.down.sql(1 hunks)
🔇 Additional comments (11)
config/makefile/helpers.mk (2)
2-4: Well-structured utility function for dependency auditing.This
external_depsmacro provides a clean way to list external dependencies for Go packages, filtering out internal ones. The command chaining is efficient and the output format is user-friendly.
7-9: Simple but useful padding utility.The
padding_lrfunction provides a clean abstraction for generating space padding, which can help with consistent output formatting across your Makefiles.database/infra/migrations/000001_schema.down.sql (1)
1-1: Correctly added drop statement for the new newsletters table.The added DROP statement aligns with the changes mentioned in the AI summary about adding a new
newsletterstable. This ensures proper cleanup during rollbacks.config/makefile/app.mk (2)
3-4: Clean implementation of Go code formatting.The
formattarget provides a straightforward way to format Go code using standard tools.
15-20: Well-structured dependency audit target.The
audittarget effectively uses theexternal_depshelper to list external dependencies for various project paths. This provides valuable insights for dependency management.config/makefile/db.mk (5)
1-24: Well-organized variables and metadata.The metadata and variable definitions are clearly structured, which enhances readability and maintainability. The logical grouping (Docker, Paths, SSL, Migrations) makes it easy to understand the different components.
25-32: Good implementation of database connection and seeding targets.The
db:localanddb:seedtargets provide clear ways to connect to the local database and seed it with data.
33-52: Well-structured database management targets.These targets (
db:up,db:ping,db:bash,db:fresh,db:logs,db:delete) provide a comprehensive set of commands for managing the database container lifecycle.
65-77: Well-implemented SSL and migration targets.The
db:secure:show,db:migrate, anddb:rollbacktargets are well-structured with clear output formatting and appropriate Docker commands.
79-80: Clear migration creation target.The
db:migrate:createtarget correctly creates a new migration file with the given name.Makefile (1)
47-92: Verify help target coverage
Thehelptarget is hand‑crafted and may drift from real targets in your modular files (e.g. Docker cleanup or audit commands inapp.mk). Please confirm that all available commands are listed under the correct headings. You might also consider dynamically generating help based on.PHONYtargets to prevent future desynchronisation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
Makefile (6)
4-4: 🧹 Nitpick (assertive)Consider using a portable SHELL definition
Hard‑coding/bin/bashmay break on systems where Bash is installed elsewhere. Useenvfor portability:-SHELL := /bin/bash +SHELL := /usr/bin/env bash
9-16: 🧹 Nitpick (assertive)Improve ANSI colour portability
Inline escape sequences can behave inconsistently across different terminals. Consider leveragingtputor honouring aNO_COLORflag:NC := $(shell tput sgr0) GREEN := $(shell tput setaf 2) # …
24-26: 🧹 Nitpick (assertive)Simplify path variables using built-in functions
Avoid spawning a subshell; useCURDIRfor the current directory and remove redundant trailing slashes:-ROOT_PATH := $(shell pwd) -APP_PATH := $(ROOT_PATH)/ +ROOT_PATH := $(CURDIR) +APP_PATH := $(CURDIR)
29-29: 🛠️ Refactor suggestionFix
REPO_OWNERcomputation
The current command returns the parent directory, not the GitHub owner. Derive it from the remote URL:-REPO_OWNER := $(shell cd .. && basename "$$(pwd)") +REPO_OWNER := $(shell git remote get-url origin | sed -E 's#.*/([^/]+)/.*#\1#')
30-30: 🧹 Nitpick (assertive)Enhance
VERSIONfallback logic
If there are no tags,git describe --tagscan return empty. Use--always --dirtyand strip a leadingv:-VERSION := $(shell git describe --tags 2>/dev/null | cut -c 2-) +VERSION := $(shell git describe --tags --always --dirty 2>/dev/null | sed 's/^v//')
35-42: 🧹 Nitpick (assertive)Annotate included makefiles
Consider adding a brief comment for eachincludeto describe its purpose and help newcomers navigate the modular structure:include ./config/makefile/helpers.mk # Helper functions (formatting, padding) include ./config/makefile/env.mk # Environment setup & verification # …
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
Makefile (1)
37-44: Document included Makefile modules.The modular structure is excellent. To help new contributors, add brief comments above each
includethat explain the responsibility of each snippet.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Makefile(1 hunks)main.go(1 hunks)
🔇 Additional comments (2)
main.go (2)
16-20: Validate environment loading and dependency setup.The
bootstrap.Spark("./.env")invocation may fail silently if the.envfile is missing or invalid. Consider returning and checking an error fromSpark, logging failure, or exiting early to prevent nil-pointer dereferences ininit().
23-27: Verify removal of Sentry recovery logic.The deferred
sentry.Recover()has been removed, yet you still initialise and passlocalSentryinto the app. Please confirm that panics and errors are still captured as intended, or update the application to align with the new error‑handling strategy.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
config/makefile/logs.mk (1)
6-7: 🧹 Nitpick (assertive)Remove extraneous empty lines.
There are multiple empty lines at the end of the file. Consider keeping just one empty line for consistency.
logs\:fresh: find $(STORAGE_PATH)/logs -maxdepth 1 -type f -not -name ".gitkeep" -delete -
♻️ Duplicate comments (10)
config/makefile/logs.mk (1)
3-4: Consider recursive log cleanup option.The current implementation only deletes files at the top level of the logs directory (due to
-maxdepth 1). If you ever need to clean up subdirectories as well, you might want to add a separate target.config/makefile/build.mk (3)
20-26: Include architecture in target name.The target builds specifically for Linux amd64, but the name only mentions Linux. Consider renaming it to be more specific.
-build\:app\:linux: +build\:app\:linux-amd64:
28-30: Define or Replace theVVariable inbuild.mk.The
build:releasetarget references$(V), but there's no definition ofVin the Makefiles. To fix this:• In
config/makefile/build.mk, either:
- Add a definition, e.g.:
(assumingV ?= $(VERSION)VERSIONis already defined elsewhere)- Or replace
$(V)with$(VERSION)directly in the recipe:build:release: git tag v$(VERSION) - @read -p "Press enter to confirm and push to origin ..." && git push origin v$(V) + @read -p "Press enter to confirm and push to origin ..." && git push origin v$(VERSION)
28-30: Add version validation.There's no validation of the version format before creating and pushing a tag. Consider adding a simple regex check to ensure the version follows semantic versioning.
build\:release: + @if ! echo "$(V)" | grep -qE '^[0-9]+\.[0-9]+\.[0-9]+$$'; then \ + echo "Error: Version format must be x.y.z (e.g., 1.2.3)"; \ + exit 1; \ + fi git tag v$(V) @read -p "Press enter to confirm and push to origin ..." && git push origin v$(V)Makefile (6)
6-6: Use a more portable SHELL definition.Hard‑coding
/bin/bashmay break on systems where Bash lives elsewhere. Consider:-SHELL := /bin/bash +SHELL := /usr/bin/env bashto ensure portability.
11-19: Improve colour‑code portability.Inline ANSI sequences can vary across terminals. You could generate colours at runtime via
tputor respect aNO_COLORtoggle:-NC := \033[0m -BOLD := \033[1m -CYAN := \033[36m -WHITE := \033[37m -GREEN := \033[0;32m -BLUE := \033[0;34m -RED := \033[0;31m -YELLOW := \033[1;33m +NC := $(shell [ -n "$$NO_COLOR" ] && echo "" || tput sgr0) +BOLD := $(shell [ -n "$$NO_COLOR" ] && echo "" || tput bold) +CYAN := $(shell [ -n "$$NO_COLOR" ] && echo "" || tput setaf 6) +WHITE := $(shell [ -n "$$NO_COLOR" ] && echo "" || tput setaf 7) +GREEN := $(shell [ -n "$$NO_COLOR" ] && echo "" || tput setaf 2) +BLUE := $(shell [ -n "$$NO_COLOR" ] && echo "" || tput setaf 4) +RED := $(shell [ -n "$$NO_COLOR" ] && echo "" || tput setaf 1) +YELLOW := $(shell [ -n "$$NO_COLOR" ] && echo "" || tput setaf 3)
26-28: Simplify path variables with Make's built‑ins.Instead of invoking
pwd, use$(CURDIR)and avoid trailing slashes:-ROOT_PATH := $(shell pwd) -APP_PATH := $(ROOT_PATH)/ +ROOT_PATH := $(CURDIR) +APP_PATH := $(ROOT_PATH)This reduces shell usage and potential path inconsistencies.
31-31: FixREPO_OWNERcomputation.The current line returns the parent directory's name (e.g. your user's home folder), not the GitHub organisation or repo owner. This will break any CI or release targets expecting the correct owner. Consider deriving it from the remote URL instead:
-REPO_OWNER := $(shell cd .. && basename "$$(pwd)") +REPO_OWNER := $(shell git remote get-url origin | sed -E 's#.*[:/](.*)/.*#\1#')
32-32: EnhanceVERSIONfallback logic.If there are no tags,
git describe --tagsyields empty. You might use the--always --dirtyflags and strip a leadingvto guarantee a value:-VERSION := $(shell git describe --tags 2>/dev/null | cut -c 2-) +VERSION := $(shell git describe --tags --always --dirty 2>/dev/null | sed 's/^v//')
37-43: Good modularisation of Makefile functionality.Breaking down the Makefile into smaller, focused modules improves maintainability and separation of concerns. Consider adding brief comments about what each included makefile is responsible for to help new developers understand the structure.
include ./config/makefile/helpers.mk include ./config/makefile/env.mk include ./config/makefile/db.mk include ./config/makefile/app.mk include ./config/makefile/logs.mk include ./config/makefile/build.mk + +# helpers.mk - Helper functions and utilities +# env.mk - Environment setup and verification +# db.mk - Database operations, migrations, and container management +# app.mk - Application formatting, watching, and dependency auditing +# logs.mk - Log cleanup and management +# build.mk - Application build and run targets
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
Makefile(1 hunks)config/makefile/build.mk(1 hunks)config/makefile/logs.mk(1 hunks)
🔇 Additional comments (2)
config/makefile/build.mk (1)
45-45: Improved security: Fixed directory permissions.Excellent improvement! The directory permissions have been changed from the overly permissive 777 to the more secure 755, which aligns with security best practices.
Makefile (1)
48-92: Comprehensive and well-organized help output.The help system is well-structured with clearly categorized commands and consistent formatting. The color-coding improves readability and the command descriptions are concise yet informative.
No description provided.