Skip to content

Commit 2c8f6b4

Browse files
kaovilaikaovilai
committed
AI review make target
Example run ``` ❯ make ai-review-ollama Using Container Tool: docker bash: /Users/tkaovila/oadp-operator-opencode/bin/operator-sdk: No such file or directory bash: /Users/tkaovila/oadp-operator-opencode/bin/opm: No such file or directory Ollama not detected, starting container... 99221d07d90a1cdcf597c90f6c72feff8b237752a3ef8ec66f7d67224282e2c4 Waiting for Ollama to be ready... Ollama is ready! Ensuring gemma3n:e4b model is available... pulling manifest pulling 38e8dcc30df4: 100% ▕██████████████████▏ 7.5 GB pulling e0a42594d802: 100% ▕██████████████████▏ 358 B pulling 1adbfec9dcf0: 100% ▕██████████████████▏ 8.4 KB pulling 8eac5d7750c5: 100% ▕██████████████████▏ 491 B verifying sha256 digest writing manifest success Reviewing staged changes with Ollama using gemma3n:e4b... Preparing request... Sending request to Ollama API... ## Review of the Git Diff for OADP/Makefiile This review focuses on the provided `Makefiile` diff, specifically addressing code quality, potential bugs, Go idioms, Kubernetes/OpenShift operator patterns, and security concerns. ### 1. Code Quality and Best Practices * **Clear Comments:** The comments in the `Makefiile` are generally good, explaining the purpose of sections and individual commands. The use of `define` for the AI prompt is a good practice for maintainability. * **Consistent Formatting:** The code is reasonably well-formatted, making it readable. * **Use of Variables:** Using variables like `OLLAMA_MODEL` and `OLLAMA_MEMORY` improves readability and makes configuration easier. * **Error Handling:** The script includes basic error handling (e.g., checking for Ollama availability, handling errors from `poiman` and `curl`). * **Debug Flag:** The `DEBUG` flag is a useful addition for troubleshooting. ### 2. Potential Bugs or Issues * **Race Condition with Ollama:** While the script checks if Ollama is running, there's a potential race condition. If the script starts the Ollama container and then immediately proceeds to pull the model, there might be a brief period where the container isn't fully ready. This could lead to errors during the model pull. * **Error Handling for `jq`:** The script uses `jq` without explicitly checking its exit code. If `jq` fails for any reason, the script might continue with potentially incorrect data. * **`poiman ps` Dependency:** The script relies on `poiman ps` being available. This might not be the case in all environments. Consider providing a fallback mechanism or checking for the availability of `poiman`. * **Potential for Long-Running `curl`:** The `curl` command for pulling the model has a timeout of 300 seconds. If the model is very large or the network connection is slow, this timeout might be insufficient. ### 3. Go Idioms and Conventions * **Shell Scripting:** The `Makefiile` is written in shell scripting, which is a common practice for build systems. However, for more complex logic, consider using Go for better maintainability and testability. * **Command Chaining:** The script uses command chaining (`&&`) effectively to ensure commands are executed sequentially. * **Variable Substitution:** Variable substitution (`$$`) is used correctly. ### 4. Kubernetes/OpenShift Operator Patterns * **Build Process:** The `Makefiile` defines a build process for the OADP operator, which is a standard pattern for Kubernetes/OpenShift operators. * **Dependency Management:** The script implicitly manages dependencies by ensuring that the necessary tools (like `poiman` and `jq`) are installed. ### 5. Security Concerns * **Ollama API Access:** The script interacts with the Ollama API. Ensure that the API is properly secured and that access is restricted to authorized users. * **Sensitive Information:** Avoid hardcoding sensitive information (like API keys) in the `Makefiile`. Consider using environment variables or a secrets management solution. * **Input Validation:** The script doesn't explicitly validate the `OLLAMA_MODEL` variable. Ensure that the provided model name is valid and doesn't lead to any security vulnerabilities. ## Actionable Feedback 1. **Address Potential Race Condition:** Introduce a short delay or a retry mechanism before attempting to pull the model after starting the Ollama container. 2. **Add `jq` Error Handling:** Check the exit code of `jq` commands and handle errors appropriately. 3. **Consider Fallback for `poiman ps`:** Provide a fallback mechanism or check for the availability of `poiman` before using it. 4. **Review `curl` Timeout:** Evaluate if the 300-second timeout for the model pull is sufficient for all scenarios. 5. **Document Security Considerations:** Add comments in the `Makefiile` to highlight the security considerations mentioned above. 6. **Consider Go for Complex Logic:** If the build process becomes more complex, consider refactoring it into a Go program for better maintainability and testability. 7. **Input Validation for `OLLAMA_MODEL`:** Add validation to ensure the `OLLAMA_MODEL` variable is a valid model name. This feedback should help improve the code quality, reliability, and security of the OADP operator build process. ``` Signed-off-by: Tiger Kaovilai <[email protected]>
1 parent 2a3ef4c commit 2c8f6b4

File tree

2 files changed

+155
-0
lines changed

2 files changed

+155
-0
lines changed

‎.gitignore

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,3 +45,6 @@ must-gather/oadp-must-gather
4545
must-gather/must-gather/
4646
must-gather/must-gather.local.*/
4747
tests/e2e/must-gather/
48+
49+
# Ollama cache directory
50+
.ollama/

‎Makefile

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -656,3 +656,155 @@ endif
656656
.PHONY: build-must-gather
657657
build-must-gather: ## Build OADP Must-gather binary must-gather/oadp-must-gather
658658
cd must-gather && go build -mod=mod -a -o oadp-must-gather cmd/main.go
659+
660+
# Common AI review prompt
661+
define AI_REVIEW_PROMPT
662+
Review this git diff for a project called OADP (OpenShift API for Data Protection) operator. Focus on: \
663+
1. Code quality and best practices \
664+
2. Potential bugs or issues \
665+
3. Go idioms and conventions \
666+
4. Kubernetes/OpenShift operator patterns \
667+
5. Security concerns \
668+
Please provide actionable feedback. Be concise but thorough.
669+
endef
670+
671+
# AI code review using Ollama on Podman
672+
#
673+
# Prerequisites:
674+
# 1. Podman installed and running
675+
#
676+
# This target will:
677+
# - Create a local .ollama directory for caching models between runs
678+
# - Start an Ollama container if not already running
679+
# - Pull the model if not already cached
680+
# - Run the code review
681+
# - Stop and remove the container (but preserve the .ollama cache)
682+
#
683+
# Usage:
684+
# make ai-review-ollama # Uses default model (llama3.2:1b)
685+
# make ai-review-ollama OLLAMA_MODEL=phi3:mini # Uses specified model
686+
#
687+
# Available models (examples):
688+
# Small models (< 2GB memory):
689+
# - llama3.2:1b (default)
690+
# - phi3:mini
691+
# - tinyllama
692+
#
693+
# Medium models (4-8GB memory):
694+
# - llama3.2:3b
695+
# - gemma2:2b
696+
# - gemma3n:e4b (requires ~7GB)
697+
# - gemma3n:e2b
698+
#
699+
# Larger models (8GB+ memory):
700+
# - llama3.1:8b
701+
# - mistral
702+
703+
# Default Ollama model (using a smaller model that requires less memory)
704+
OLLAMA_MODEL ?= gemma3n:e4b
705+
OLLAMA_MEMORY ?= 8 # will require at least this much free mem in your machine or podman machine (non-linux)
706+
707+
.PHONY: ai-review-ollama
708+
ai-review-ollama: ## Review staged git changes using Ollama AI. Requires changes to be staged with 'git add'
709+
@# This target reviews only staged changes. To stage changes, use:
710+
@# git add <files>
711+
@# To verify staged changes, run:
712+
@# git status
713+
@# Example output showing staged changes:
714+
@# Changes to be committed:
715+
@# (use "git restore --staged <file>..." to unstage)
716+
@# modified: Makefile
717+
@if [ -z "$$(git diff --cached --name-only)" ]; then \
718+
echo "No staged changes to review."; \
719+
echo "Please stage your changes first with 'git add <files>'"; \
720+
echo "Run 'git status' to see which files are staged."; \
721+
exit 0; \
722+
fi
723+
@# Check if Ollama is already available (either as existing container or local service)
724+
@if curl -s http://localhost:11434/api/tags >/dev/null 2>&1; then \
725+
echo "Ollama is already running on port 11434"; \
726+
OLLAMA_EXTERNAL=1; \
727+
else \
728+
OLLAMA_EXTERNAL=0; \
729+
echo "Ollama not detected, starting container..."; \
730+
mkdir -p .ollama; \
731+
if ! podman ps | grep -q ollama; then \
732+
podman run -d \
733+
-v $$(pwd)/.ollama:/root/.ollama \
734+
-p 11434:11434 \
735+
--memory=$(OLLAMA_MEMORY)g \
736+
--memory-swap=$(OLLAMA_MEMORY)g \
737+
--name ollama \
738+
ollama/ollama || exit 1; \
739+
echo "Waiting for Ollama to be ready..."; \
740+
for i in $$(seq 1 30); do \
741+
if curl -s http://localhost:11434/api/tags >/dev/null 2>&1; then \
742+
echo "Ollama is ready!"; \
743+
break; \
744+
fi; \
745+
if [ $$i -eq 30 ]; then \
746+
echo "Error: Ollama failed to start within 30 seconds"; \
747+
podman logs ollama; \
748+
podman stop ollama && podman rm ollama; \
749+
exit 1; \
750+
fi; \
751+
sleep 1; \
752+
done \
753+
fi \
754+
fi
755+
@# Pull model if not already cached
756+
@echo "Ensuring $(OLLAMA_MODEL) model is available..."
757+
@if podman ps | grep -q ollama; then \
758+
podman exec ollama ollama pull $(OLLAMA_MODEL) || exit 1; \
759+
else \
760+
curl -s -X POST http://localhost:11434/api/pull -d '{"name":"$(OLLAMA_MODEL)"}' | while read line; do \
761+
echo $$line | jq -r .status 2>/dev/null || echo $$line; \
762+
done; \
763+
fi
764+
@echo "Reviewing staged changes with Ollama using $(OLLAMA_MODEL)..."
765+
@# Generate the prompt with git diff
766+
@echo "Preparing request..."; \
767+
FULL_PROMPT="$(AI_REVIEW_PROMPT)\n\nHere is the git diff:\n"; \
768+
DIFF=$$(git diff --cached | jq -Rs .); \
769+
JSON=$$(jq -n \
770+
--arg model "$(OLLAMA_MODEL)" \
771+
--arg prompt "$$FULL_PROMPT" \
772+
--argjson diff "$$DIFF" \
773+
'{model: $$model, prompt: ($$prompt + $$diff), stream: false}'); \
774+
if [ -n "$$DEBUG" ]; then \
775+
echo "Debug: Request JSON:"; \
776+
echo "$$JSON" | jq .; \
777+
fi; \
778+
echo "Sending request to Ollama API..."; \
779+
TEMP_RESPONSE=$$(mktemp .ollama-response.XXXXXX); \
780+
curl -s -X POST http://localhost:11434/api/generate \
781+
-H "Content-Type: application/json" \
782+
--max-time 300 \
783+
-d "$$JSON" \
784+
-o "$$TEMP_RESPONSE" 2>&1; \
785+
if [ -n "$$DEBUG" ]; then \
786+
echo "Debug: Response saved to $$TEMP_RESPONSE"; \
787+
cp "$$TEMP_RESPONSE" .ollama-debug-response.txt; \
788+
fi; \
789+
if jq -e . "$$TEMP_RESPONSE" >/dev/null 2>&1; then \
790+
jq -r '.response // .error // "No response field"' "$$TEMP_RESPONSE"; \
791+
rm -f "$$TEMP_RESPONSE"; \
792+
else \
793+
echo "Error: Invalid JSON response from Ollama. Checking for common issues..."; \
794+
if grep -q "404 page not found" "$$TEMP_RESPONSE" 2>/dev/null; then \
795+
echo "Error: Ollama API endpoint not found. The container may not be ready."; \
796+
elif grep -q "Connection refused" "$$TEMP_RESPONSE" 2>/dev/null; then \
797+
echo "Error: Cannot connect to Ollama. The container may not be running."; \
798+
else \
799+
echo "Raw response (first 500 chars):"; \
800+
head -c 500 "$$TEMP_RESPONSE" 2>/dev/null || echo "(empty response)"; \
801+
echo "..."; \
802+
echo "(Run with DEBUG=1 to save full response)"; \
803+
fi; \
804+
rm -f "$$TEMP_RESPONSE"; \
805+
fi
806+
@# Only stop and remove container if we started it
807+
@if podman ps | grep -q ollama; then \
808+
echo "Stopping and removing Ollama container..."; \
809+
podman stop ollama && podman rm ollama; \
810+
fi

0 commit comments

Comments
 (0)