-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(functions): add remoteSource utility and path resolution helper #9077
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
|
/gemini review |
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.
Code Review
This pull request introduces a new utility for cloning remote Git repositories to deploy functions, along with a helper for secure path resolution. The implementation is robust, with good error handling, retry logic for network operations, and comprehensive tests. I've provided a couple of suggestions to refactor parts of the remoteSource.ts file to improve code clarity and maintainability.
|
/gemini review |
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.
Code Review
This pull request introduces a utility to clone remote Git repositories for deploying functions, along with a path resolution helper. The implementation is well-structured, using a GitClient interface for testability and providing helpful error messages for common failures. My review identified two high-severity issues: a potential bug in error handling where the original error object could be lost, making debugging difficult, and a lack of test coverage for the success paths of the main cloneRemoteSource function. Addressing these points will improve the robustness and reliability of this new feature.
779cf15 to
8dbca4b
Compare
8dbca4b to
920b86f
Compare
|
/gemini review |
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.
Code Review
This pull request introduces new utility functions for handling remote Git sources for function deployments. It adds cloneRemoteSource to clone a repository, along with a GitClient interface and a default implementation using spawnSync. It also includes a path helper resolveWithin to securely resolve subdirectories within a repository, preventing path traversal vulnerabilities. The changes are well-tested with new spec files for both remoteSource and pathUtils.
My review includes a couple of suggestions to improve code style and reduce duplication in the new remoteSource.ts file. Overall, the implementation is solid and the error handling is comprehensive.
Co-authored-by: Jeff <3759507+jhuleatt@users.noreply.github.com>
…ls` and updating `remoteSource` to use it.
…e/firebase-tools into pr-remote-src-pr2-master
Add utility function required for remote source implementations.