-
Notifications
You must be signed in to change notification settings - Fork 180
feat: implement recursive CRD discovery and robust GitHub URL parsing #887
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
base: master
Are you sure you want to change the base?
feat: implement recursive CRD discovery and robust GitHub URL parsing #887
Conversation
…rent /tmp sub folder instead of cwd Signed-off-by: Muhammad Akewukanwo <[email protected]>
…kipped file Signed-off-by: Muhammad Akewukanwo <[email protected]>
Signed-off-by: Muhammad Akewukanwo <[email protected]>
Signed-off-by: Muhammad Akewukanwo <[email protected]>
Signed-off-by: Muhammad Akewukanwo <[email protected]>
Signed-off-by: Muhammad Akewukanwo <[email protected]>
Signed-off-by: Muhammad Akewukanwo <[email protected]>
Signed-off-by: Muhammad Akewukanwo <[email protected]>
Signed-off-by: Muhammad Akewukanwo <[email protected]>
Signed-off-by: Muhammad Akewukanwo <[email protected]>
Signed-off-by: Muhammad Akewukanwo <[email protected]>
Signed-off-by: Muhammad Akewukanwo <[email protected]>
Signed-off-by: Muhammad Akewukanwo <[email protected]>
Summary of ChangesHello @muhammadolammi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the model generation process by introducing a more resilient and intelligent approach to handling GitHub source URLs. It allows for recursive CRD discovery by supporting diverse URL formats, including browser and release links, and automatically resolves default branches when not explicitly provided. These changes, coupled with improved zip extraction security, contribute to a more robust and flexible system for fetching and processing CRDs. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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 more robust mechanism for parsing various GitHub URL formats and implements recursive discovery for CRDs. A significant improvement is the rewrite of the ExtractZip function, which now correctly handles resource cleanup, prevents Zip Slip security vulnerabilities, and extracts files to the correct destination. The accompanying tests are comprehensive. My feedback focuses on a critical edge case in the new URL parsing logic that could lead to a panic, a minor typo, and a suggestion to improve test hermeticity by mocking network calls.
| "testing" | ||
| ) | ||
|
|
||
| func TestExtractRepoDetailsFromSourceURL(t *testing.T) { |
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.
This test makes real network calls to the GitHub API when a URL with only an owner and repo is provided (e.g., git://github.com/meshery/meshkit). This makes the test non-hermetic, slow, and potentially flaky, depending on network conditions and the state of the remote repository (e.g., if the default branch changes).
Unit tests should be self-contained and not rely on external services. The call to GetDefaultBranchFromGitHub within extractRepoDetailsFromSourceURL should be mocked.
One way to achieve this is to introduce a function variable in git_repo.go that can be replaced during tests.
In generators/github/git_repo.go:
// Add this variable at the package level
var getDefaultBranch = GetDefaultBranchFromGitHub
// In extractRepoDetailsFromSourceURL, change the call to use this variable
// ...
b, err := getDefaultBranch("https://api.github.com", owner, repo, http.DefaultClient)
// ...Then, in generators/github/git_repo_test.go, you can mock it:
func TestExtractRepoDetailsFromSourceURL(t *testing.T) {
// Mock the function
oldGetDefaultBranch := getDefaultBranch
getDefaultBranch = func(baseUrl, owner, repo string, client *http.Client) (string, error) {
// For this test, we can assume the default branch is always "master"
// to match the test expectations.
if owner == "meshery" && repo == "meshkit" {
return "master", nil
}
return "", fmt.Errorf("unexpected repo: %s/%s", owner, repo)
}
// Restore the original function after the test
defer func() { getDefaultBranch = oldGetDefaultBranch }()
// ... rest of the test code
}This change will make your tests fast, reliable, and independent of external services.
Signed-off-by: Muhammad Akewukanwo <[email protected]>
149bdd0 to
0225ce3
Compare
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Aabid Sofi <[email protected]>
|
@Yorisoft, would you like to give this PR a review? |
|
@parthivsaikia, and you? |
|
@muhammadolammi I would suggest less comments if possible: code should be self explaining. As I'm still learning how things work I can't give a garuntee that this works as intended. I will give it another pass to validate. Hopefully you get others to review too. |
|
This is a good item to add to the weekly Meshery Dev meeting agenda. You can add this item in the doc, attend, and present it. Meeting details at https://meshery.io/calendar. |
Description
This PR fixes #881
This PR introduces a more resilient approach to model generation by implementing recursive tree walking and intelligent URL routing.
Testing Performed
A comprehensive test suite was added to package_test.go c:
[x] Standard git:// protocol with specific roots.
[x] GitHub Browser URLs containing /tree/ and /blob/.
[x] GitHub Release Tag URLs (e.g., /releases/tag/v1.0).
[x] Root-level discovery with auto-branch resolution.
[x] Handling of "messy" URLs with multiple slashes and trailing separators.
Notes for Reviewers
This PR depends on #885
Signed commits