Skip to content

chore: use makefiles #3453

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.

Sign up for GitHub

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

Merged
chronark merged 22 commits into from
Jul 7, 2025
Merged

chore: use makefiles #3453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
4fdac11
feat: boilerplate for controlplane service
chronark Jul 1, 2025
cbfc389
feat: control plane and cli demo
chronark Jul 2, 2025
558fb09
chore: merge main
chronark Jul 2, 2025
ab13738
chore: refactor gorm to sqlc
chronark Jul 2, 2025
ee8462d
Revert "chore: refactor gorm to sqlc"
chronark Jul 3, 2025
a7c146c
feat: mocked buildd
chronark Jul 3, 2025
b9c3eaf
Merge branch 'main' of https://github.com/unkeyed/unkey into controlp…
chronark Jul 3, 2025
26feb93
feat: use hydra for the deployment orchestration
chronark Jul 3, 2025
31dba4b
feat: migrate hydra sqlc schema
chronark Jul 3, 2025
7303653
chore: setup dualstore
chronark Jul 4, 2025
b0f7030
chore: migrated GetWorkflow
chronark Jul 4, 2025
1297aa6
chore: finish migration
chronark Jul 4, 2025
b091cf4
chore: migrate more shit
chronark Jul 4, 2025
b73ecb4
fix: sqlc
chronark Jul 4, 2025
a969578
chore: clean up everything
chronark Jul 7, 2025
119f75d
chore: remove binary
chronark Jul 7, 2025
4d55f46
Merge branch 'main' into controlplane
chronark Jul 7, 2025
ba42153
test: unify schema migration to run at container startup
chronark Jul 7, 2025
7f929e8
chore: use makefile
chronark Jul 7, 2025
28a7798
Merge branch 'main' of https://github.com/unkeyed/unkey into makefile
chronark Jul 7, 2025
483979a
Merge branch 'main' of https://github.com/unkeyed/unkey into makefile
chronark Jul 7, 2025
6b95dbf
chore: clean up
chronark Jul 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
.PHONY: pull build down migrate-clickhouse migrate-clickhouse-reset integration generate-sql nuke-docker
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 7, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add standard Makefile targets.

The static analysis correctly identifies missing standard targets. Consider adding them for better Makefile conventions.

-.PHONY: pull build down migrate-clickhouse migrate-clickhouse-reset integration generate-sql nuke-docker
+.PHONY: all clean test pull build down migrate-clickhouse migrate-clickhouse-reset integration generate-sql nuke-docker
+
+all: build
+
+clean: down
+	docker system prune -f
+
+test: integration
🧰 Tools
🪛 checkmake (0.2.2)

[warning] 1-1: Missing required phony target "all"

(minphony)

[warning] 1-1: Missing required phony target "clean"

(minphony)

[warning] 1-1: Missing required phony target "test"

(minphony)

🤖 Prompt for AI Agents 
In the Makefile at line 1, standard Makefile targets are missing which are
important for conventional usage. Add common targets such as 'clean', 'test',
and 'install' to the .PHONY list and define their corresponding commands to
improve the Makefile's usability and maintainability.


pull:
docker compose -f ./deployment/docker-compose.yaml pull

build: pull
docker compose -f ./deployment/docker-compose.yaml build

down:
docker compose -f ./deployment/docker-compose.yaml down

up: down build
docker compose -f ./deployment/docker-compose.yaml up -d

migrate-clickhouse:
@export GOOSE_DRIVER=clickhouse && \
export GOOSE_DBSTRING="tcp://default:password@127.0.0.1:9000" && \
export GOOSE_MIGRATION_DIR=./internal/clickhouse/schema && \
goose up
Comment on lines +16 to +19
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 7, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid hardcoded database connection strings.

The ClickHouse connection string is hardcoded with credentials. Consider using environment variables or a configuration file.

 migrate-clickhouse:
-	@export GOOSE_DRIVER=clickhouse && \
-	export GOOSE_DBSTRING="tcp://default:password@127.0.0.1:9000" && \
-	export GOOSE_MIGRATION_DIR=./internal/clickhouse/schema && \
-	goose up
+	@export GOOSE_DRIVER=clickhouse && \
+	export GOOSE_DBSTRING="$${CLICKHOUSE_URL:-tcp://default:password@127.0.0.1:9000}" && \
+	export GOOSE_MIGRATION_DIR=./internal/clickhouse/schema && \
+	goose up
🤖 Prompt for AI Agents 
In the Makefile at lines 16 to 19, the ClickHouse connection string is hardcoded
with credentials, which is insecure. Replace the hardcoded connection string
with a reference to environment variables for the username, password, host, and
port. Update the export command to build the GOOSE_DBSTRING dynamically from
these environment variables or load them from a configuration file, ensuring no
sensitive data is directly in the Makefile.


migrate-clickhouse-reset:
@export GOOSE_DRIVER=clickhouse && \
export GOOSE_DBSTRING="tcp://default:password@127.0.0.1:9000" && \
export GOOSE_MIGRATION_DIR=./internal/clickhouse/schema && \
goose down-to 0

integration: up
@cd apps/api && \
$(MAKE) seed && \
pnpm test:integration

generate-sql:
@cd internal/db && \
pnpm drizzle-kit generate --dialect=mysql

nuke-docker:
docker stop $$(docker ps -aq)
docker system prune -af
docker volume prune --all -f
Comment on lines +36 to +39
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 7, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Make the nuke-docker target safer.

The nuke-docker target is very aggressive and could accidentally remove unrelated containers and data. Consider adding safeguards or warnings.

 nuke-docker:
-	docker stop $$(docker ps -aq)
-	docker system prune -af
-	docker volume prune --all -f
+	@echo "WARNING: This will stop ALL containers and remove ALL unused Docker data!"
+	@read -p "Are you sure? [y/N] " -n 1 -r && echo && \
+	if [[ $$REPLY =~ ^[Yy]$$ ]]; then \
+		docker stop $$(docker ps -aq) 2>/dev/null || true; \
+		docker system prune -af; \
+		docker volume prune --all -f; \
+	fi
🤖 Prompt for AI Agents 
In the Makefile around lines 36 to 39, the nuke-docker target aggressively stops
all containers and prunes all system and volume data without any confirmation,
which risks accidental data loss. Modify the target to include a confirmation
prompt before executing the destructive commands, or require an explicit
argument or environment variable to proceed, ensuring the user acknowledges the
risk before the commands run.

63 changes: 0 additions & 63 deletions Taskfile.yml
View file
Open in desktop

This file was deleted.

26 changes: 26 additions & 0 deletions apps/agent/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
.PHONY: install fmt test build race lint generate

Comment on lines +1 to +2
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 7, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Missing standard phony entry points (all, clean).

checkmake rightfully flags the absence of all and clean targets. Including them keeps the interface of all repo Makefiles uniform and prevents CI jobs or local scripts that expect these targets from breaking.

-.PHONY: install fmt test build race lint generate
+.PHONY: all clean install fmt test build race lint generate
+
+# Default target
+all: build
+
+# Remove build artefacts
+clean:
+	@rm -f unkey
📝 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.

Suggested change
.PHONY: install fmt test build race lint generate
.PHONY: all clean install fmt test build race lint generate
# Default target
all: build
# Remove build artefacts
clean:
@rm -f unkey
🧰 Tools
🪛 checkmake (0.2.2)

[warning] 1-1: Missing required phony target "all"

(minphony)

[warning] 1-1: Missing required phony target "clean"

(minphony)

🤖 Prompt for AI Agents 
In apps/agent/Makefile at lines 1 to 2, add standard phony targets `all` and
`clean` to the .PHONY declaration and define these targets. The `all` target
should typically depend on the main build or install targets, and the `clean`
target should remove generated files or build artifacts. This will align the
Makefile with repo conventions and prevent CI or scripts from failing due to
missing these common targets.

install:
go mod tidy

fmt: lint
go fmt ./...

test:
go test -cover -json -failfast ./... | tparse -all -progress

Comment on lines +9 to +11
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 7, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

tparse is required but never installed – tests will fail on fresh machines.

Either vendor tparse in the repo or add an explicit go install before use to avoid “command not found”.

test: | tools
	go test -cover -json -failfast ./... | tparse -all -progress

+# Ensure helpers are present
+tools:
+	go install github.com/mfridman/tparse@latest
🤖 Prompt for AI Agents 
In apps/agent/Makefile around lines 9 to 11, the test target uses the `tparse`
command which is not guaranteed to be installed on fresh machines, causing test
failures. Fix this by adding a step before running tests to explicitly install
`tparse` using `go install` or by vendoring `tparse` in the repository to ensure
it is always available when running tests.

build:
go build -o unkey ./cmd/main.go

race:
go install github.com/amit-davidson/Chronos/cmd/chronos
~/go/bin/chronos --file=./cmd/main.go --mod=$$(pwd)

Comment on lines +15 to +18
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 7, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Hard-coded ~/go/bin breaks for users with a custom GOPATH.

Derive the bin path from go env GOPATH instead of assuming ~/go.

-	~/go/bin/chronos --file=./cmd/main.go --mod=$$(pwd)
+	$$(go env GOPATH)/bin/chronos --file=./cmd/main.go --mod=$$(pwd)
📝 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.

Suggested change
race:
go install github.com/amit-davidson/Chronos/cmd/chronos
~/go/bin/chronos --file=./cmd/main.go --mod=$$(pwd)
race:
go install github.com/amit-davidson/Chronos/cmd/chronos
$$(go env GOPATH)/bin/chronos --file=./cmd/main.go --mod=$$(pwd)
🤖 Prompt for AI Agents 
In apps/agent/Makefile around lines 15 to 18, the path to the chronos binary is
hard-coded as ~/go/bin, which fails for users with a custom GOPATH. Replace the
hard-coded path with a dynamic one by using the output of `go env GOPATH` to
construct the binary path, ensuring it works regardless of the user's GOPATH
configuration.

lint:
golangci-lint run

generate:
go get github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen
mkdir -p ./pkg/openapi
go run github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen --config=./pkg/openapi/config.yaml ./pkg/openapi/openapi.json
buf generate
Comment on lines +22 to +26
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 7, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

go get mutates go.mod; switch to version-pinned go install.

Since Go 1.20 go get in build scripts is deprecated and pollutes the module graph. Use go install <pkg>@<version> for reproducible builds.

-	go get github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen
+	go install github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen@v2.8.0
🤖 Prompt for AI Agents 
In apps/agent/Makefile around lines 22 to 26, replace the use of 'go get' with
'go install' including a specific version suffix to avoid mutating go.mod and
ensure reproducible builds. Change the command to 'go install
github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen@<version>' where
<version> is the desired version tag or commit hash.

34 changes: 0 additions & 34 deletions apps/agent/Taskfile.yml
View file
Open in desktop

This file was deleted.

34 changes: 34 additions & 0 deletions go/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
.PHONY: install fmt test-unit test-full build generate lint pull build-docker
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 7, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Add standard Makefile targets for better convention compliance.

Consider adding these standard targets to improve Makefile conventions:

-.PHONY: install fmt test-unit test-full build generate lint pull build-docker
+.PHONY: all clean test install fmt test-unit test-full build generate lint pull build-docker
+
+all: build
+
+clean:
+	go clean
+	rm -f unkey
+
+test: test-unit
📝 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.

Suggested change
.PHONY: install fmt test-unit test-full build generate lint pull build-docker
.PHONY: all clean test install fmt test-unit test-full build generate lint pull build-docker
all: build
clean:
go clean
rm -f unkey
test: test-unit
🧰 Tools
🪛 checkmake (0.2.2)

[warning] 1-1: Missing required phony target "all"

(minphony)

[warning] 1-1: Missing required phony target "clean"

(minphony)

[warning] 1-1: Missing required phony target "test"

(minphony)

🤖 Prompt for AI Agents 
In go/Makefile at line 1, add standard Makefile targets such as 'clean', 'all',
and 'help' to improve convention compliance and usability. Define these targets
with appropriate commands or dependencies to provide common build and
maintenance operations, ensuring the Makefile follows typical project standards.


install:
go mod tidy

fmt: lint
@go fmt ./...

pull:
docker pull mysql:latest
docker pull redis:latest
docker pull grafana/otel-lgtm:latest

build-docker:
docker build -t apiv2:latest .

test-full: pull build-docker
@export INTEGRATION_TEST=true && \
export SIMULATION_TEST=false && \
echo "Running full tests... this can take more than 30min... run 'make test-unit' for faster tests" && \
go test -failfast -timeout=60m -shuffle=on -v -json ./... | tparse -all -progress -smallscreen

test-unit:
go test -json -race -failfast -timeout=30m ./... | tparse -all -progress -smallscreen

build:
go build -o unkey ./main.go

generate:
go generate ./...
# buf generate

lint:
@golangci-lint run
49 changes: 0 additions & 49 deletions go/Taskfile.yml
View file
Open in desktop

This file was deleted.

Loading
Loading