-
Notifications
You must be signed in to change notification settings - Fork 22
[Chore] Error message on missing API key for forks (CF-675) #371
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.
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
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
…-on-missing-key-in-fork`) Here’s a **faster** version, making these improvements. - Avoid repeatedly calling `os.getenv()` and `Path.open()` for the same event file, by using an explicit cache variable for the file contents. This accelerates repeated lookups and reduces disk accesses. - Remove double @lru_cache use, as the value from `get_cached_gh_event_data()` will not change for a process lifetime and can be memory-cached explicitly. - Microoptimize the `is_repo_a_fork` logic by removing an unnecessary bool(...) coercion (the value is already True/False). Here’s the rewritten program. **Key notes:** - The event file is read at most once per process. - All redundant calls and mutable global state are avoided. - Fast subsequent access due to a module-level cache; you’ll see lower file-system and JSON load latency. - The returned values are identical for the same environment and state, and function signatures are maintained. Let me know if you want further micro-optimizations!
⚡️ Codeflash found optimizations for this PR📄 40% (0.40x) speedup for
|
…hore/error-on-missing-key-in-fork`) Here’s a faster and more memory-efficient version of your program. **Optimization rationale:** - Avoids the extra `Path` object creation and `.open()` call—using `open(event_path)` is faster for a single use. - Reduces dependency usage by not involving `Path` unless file system path manipulation is needed. - Leaves error handling as originally absent, maintaining original logic. - Leaves `lru_cache` as is and all function/return types unchanged. **Summary of change:** Direct file open instead of through `Path`, removing unnecessary abstraction for a single read and reducing overhead. All logic and return values are identical.
| if is_repo_a_fork(): | ||
| msg = ( | ||
| "Codeflash API key not detected in your environment. It appears you're running Codeflash from a GitHub fork.\n" | ||
| "For external contributors, please ensure you've added your own API key to your fork's repository secrets and set it as the CODEFLASH_API_KEY environment variable.\n" | ||
| f"{api_secret_docs_message}" | ||
| ) | ||
| raise OSError(msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use codeflash/code_utils/code_utils.py::exit_with_message
do we want to exit with an error code since that will mark the GHA run as failed? @misrasaurabh1 thoughts there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mohammedahmed18 meant to request changes here, not approve, make the changes please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

User description
PR Type
Enhancement
Description
Enhance API key error messaging for forks
Add fork detection via GITHUB_EVENT_PATH
Include docs link in error message
Import json and adjust type hints
Changes walkthrough 📝
env_utils.py
Fork detection and improved API key errorscodeflash/code_utils/env_utils.py