AI review make target #1817
Open
AI review make target #1817
+174
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…d updating model configurations Signed-off-by: Tiger Kaovilai <[email protected]>
…he AI review target Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
```
❯ make ai-review-gptme-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
gptme is already installed at /Users/tkaovila/oadp-operator-opencode/bin/gptme
Ollama not detected, starting container...
c3a66f9c2d48dd76b73790657f8f35a17323767dc8be9272640e960af1ad2251
Waiting for Ollama to be ready...
Ollama is ready!
Ensuring gemma3:12b model is available...
pulling manifest
pulling e8ad13eff07a: 100% ▕██████████████████▏ 8.1 GB
pulling e0a42594d802: 100% ▕██████████████████▏ 358 B
pulling dd084c7d92a3: 100% ▕██████████████████▏ 8.4 KB
pulling 3116c5225075: 100% ▕██████████████████▏ 77 B
pulling 6819964c2bcf: 100% ▕██████████████████▏ 490 B
verifying sha256 digest
writing manifest
success
Reviewing staged changes with gptme using Ollama model: gemma3:12b...
gptme will be able to read files and browse documentation for context.
[21:14:27] Browser tool available (using lynx)
[21:14:27] WARNING Skipping all confirmation prompts. cli.py:176
Using model: local/gemma3:12b
Using logdir ~/.local/share/gptme/logs/2025-07-01-sneaking-crazy-pelican
Using workspace at ~/oadp-operator-opencode
Skipped 1 hidden system messages, show with --show-hidden
--- ^^^ past messages ^^^ ---
User:
Review this git diff for a project called OADP (OpenShift API for Data Protection) operator. Focus on: 1. Code quality and best practices 2. Potential bugs or issues
3. Go idioms and conventions 4. Kubernetes/OpenShift operator patterns 5. Security concerns Please provide actionable feedback. Be concise but thorough. If able,
browse linked URLs for context.
```stdin
diff --git a/Makefile b/Makefile
index 2da6e68..c5e7d35 100644
--- a/Makefile
+++ b/Makefile
@@ -704,7 +704,7 @@ endef
# Default Ollama model (using a smaller model that requires less memory)
OLLAMA_MODEL ?= gemma3:12b
# will require at least this much free mem in your machine or podman machine (non-linux)
-OLLAMA_MEMORY ?= 9
+OLLAMA_MEMORY ?= 11
# This target reviews staged changes using gptme with Ollama backend
# Prerequisites:
```
Assistant:
Okay, I will review the provided `Makefile` diff for the OADP operator project, focusing on the requested areas.
`<thinking>` The diff is very small, only changing a variable `OLLAMA_MEMORY` from 9 to 11. This suggests a change related to resource requirements for the GPTME integration. I'm going to focus my review on the implications of this change and the surrounding context.`</thinking>
**Review of `Makefile` Diff**
* **Code Quality & Best Practices:** The change itself is trivial and doesn't introduce any immediate code quality concerns. However, the variable name `OLLAMA_MEMORY` is not self-documenting. A comment explaining *why* the value was increased would be beneficial.
* **Potential Bugs/Issues:** Increasing `OLLAMA_MEMORY` without understanding the root cause of the original value (9) could mask an underlying problem. It's possible the original value was artificially low due to a misconfiguration or resource constraint. It's crucial to investigate *why* more memory is now needed.
* **Go Idioms & Conventions:** Not applicable, as this is a Makefile, not Go code.
* **Kubernetes/OpenShift Operator Patterns:** Not applicable.
* **Security Concerns:** The change itself doesn't introduce direct security concerns. However, if the increased memory allocation leads to resource exhaustion, it *could* create a denial-of-service vulnerability. Monitoring resource usage after this change is important.
**Actionable Feedback:**
1. **Add a comment:** Add a comment to the `Makefile` explaining *why* `OLLAMA_MEMORY` was increased from 9 to 11. For example: `# Increased OLLAMA_MEMORY to 11 to resolve memory allocation issues with larger prompts.`
2. **Investigate Root Cause:** Determine why the original value of 9 was insufficient. Was it a genuine resource constraint, or a configuration issue?
3. **Monitor Resource Usage:** After deploying with the new `OLLAMA_MEMORY` value, monitor the resource usage of the GPTME integration to ensure it's stable and doesn't lead to resource exhaustion.
Stopping and removing Ollama container...
ollama
ollama
```
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
weshayutin
reviewed
Jul 2, 2025
weshayutin
reviewed
Jul 2, 2025
Makefile
Outdated
|
|
||
| # Common AI review prompt | ||
| define AI_REVIEW_PROMPT | ||
| Review this git diff for a project called OADP (OpenShift API for Data Protection) operator. Focus on: \ |
There was a problem hiding this comment.
Start small please, this prompt is TOO general, and too big.
Let's get feedback from the team with regards to what would be smart to focus on first.
Even with 11G of ram provided, this is quite slow.
IMHO a more effective first pass, MVP would be much faster and focus on one or two items of code quality. IMHO the team should discuss and pick one or two items together.
There was a problem hiding this comment.
Having a smaller prompt won't speed this up, it all depends on how big the diff is being passed and the machine..
and now you know why local models won't ever work to help day to day coding. You'll spend all day waiting without H100.
There was a problem hiding this comment.
Or be stuck with a dumber small models that can't do anything more complex than one line tab completion.
|
my resutls Assistant: Overall Impression: The changes aim to improve the reliability of starting the Ollama container for AI review. The previous implementation had some race conditions and wasn't robust. The new version addresses these issues, but there's still room for improvement. 1. Code Quality and Best Practices
2. Potential Bugs or Issues
3. Go Idioms and Conventions
4. Kubernetes/OpenShift Operator Patterns
5. Security Concerns
Actionable Feedback (with Patch) Here's a patch incorporating the suggested improvements: --- a/Makefile
+++ b/Makefile
@@ -740,36 +740,36 @@ ai-review-gptme-ollama: gptme ## Review staged git changes using gptme with loca
@# Check if Ollama is already available (either as existing container or local service)
@if curl -s http://localhost:11434/api/tags >/dev/null 2>&1; then \
echo "Ollama is already running on port 11434"; \
- OLLAMA_EXTERNAL=1; \
else \
- OLLAMA_EXTERNAL=0; \
echo "Ollama not detected, starting container..."; \
- mkdir -p .ollama; \
- if ! podman ps | grep -q ollama; then \
- podman run -d \
- -v $$(pwd)/.ollama:/root/.ollama \
- -p 11434:11434 \
- --memory=$(OLLAMA_MEMORY)g \
- --memory-swap=$(OLLAMA_MEMORY)g \
- --name ollama \
- ollama/ollama || exit 1; \
- echo "Waiting for Ollama to be ready..."; \
- for i in $$(seq 1 30); do \
- if curl -s http://localhost:11434/api/tags >/dev/null 2>&1; then \
- echo "Ollama is ready!"; \
- break; \
- fi; \
- if [ $$i -eq 30 ]; then \
- echo "Error: Ollama failed to start within 30 seconds"; \
- podman logs ollama; \
- podman stop ollama && podman rm ollama; \
- exit 1; \
- fi; \
- sleep 1; \
- done \
- fi \
+ fi
+ @# Start Ollama container if not already running
+ @if ! curl -s http://localhost:11434/api/tags >/dev/null 2>&1 && ! podman ps | grep -q ollama; then \
+ podman run -d \
+ -v $${PWD}/.ollama:/root/.ollama:Z \
+ -p 11434:11434 \
+ --memory=$(OLLAMA_MEMORY)g \
+ --memory-swap=$(OLLAMA_MEMORY)g \
+ --name ollama \
+ docker.io/ollama/ollama || exit 1; \
+ fi
+ @# Wait for Ollama to be ready
+ @if ! curl -s http://localhost:11434/api/tags >/dev/null 2>&1; then \
+ echo "Waiting for Ollama to be ready..."; \
+ for i in $$(seq 1 30); do \
+ if curl -s http://localhost:11434/api/tags >/dev/null 2>&1; then \
+ echo "Ollama is ready!"; \
+ break; \
+ fi; \
+ if [ $$i -eq 30 ]; then \
+ echo "Error: Ollama failed to start within 30 seconds"; \
+ podman logs ollama; \
+ podman stop ollama && podman rm ollama; \
+ exit 1; \
+ fi; \
+ sleep 1; \
+ done; \
fi
@# Pull model if not already cached
@echo "Ensuring $(GPTME_OLLAMA_MODEL) model is available..."This patch incorporates the suggested changes, including using |
- Move AI_REVIEW_PROMPT definition from Makefile to ai/Makefile/Prompt/prompt.example - Add conditional include logic to use custom prompt if ai/Makefile/Prompt/prompt exists - Falls back to prompt.example if custom prompt not defined - Allows users to customize AI review prompts without modifying Makefile 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
…Ollama volume mount Signed-off-by: Tiger Kaovilai <[email protected]>
|
@kaovilai: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@kaovilai so I think you have this in a place to share w/ the larger group tomorrow at scrum if you are ok w/ that. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why the changes were made
This PR introduces AI-powered code review capabilities for the OADP operator project, with two main features:
1. AI Review Make Target
New make target
ai-review-gptme-ollamathat enables automated code review using AI models locally with gptme and Ollama backend. This feature provides enhanced review capabilities with tools support.This helps developers:
2. Configurable AI Review Prompts
The AI review prompt is now externalized and configurable:
ai/Makefile/Prompt/prompt.exampleai/Makefile/Prompt/promptThe implementation uses Ollama running in a container to provide AI analysis of git diffs, focusing on:
How to test the changes made
Prerequisites:
pip install gptme)Running AI Review:
Stage your changes:
Run the AI review with gptme:
Custom Prompts:
Create a custom prompt:
cp ai/Makefile/Prompt/prompt.example ai/Makefile/Prompt/prompt # Edit ai/Makefile/Prompt/prompt to customizeRun AI review with your custom prompt:
The command will:
Options:
OLLAMA_MODEL=<model>: Use a different Ollama model (default: gemma3:12b)OLLAMA_MEMORY=<size>: Set memory limit for Ollama container (default: 11g)DEBUG=1: Enable debug outputExample output:
Available Models:
Small models (< 2GB memory):
Medium models (4-8GB memory):
Larger models (8GB+ memory):
Signed-off-by: Tiger Kaovilai [email protected]