Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Teach Updater to Invoke fetch files Based on Multi-Dir #8309

Merged
merged 22 commits into from
Nov 13, 2023
Merged

Teach Updater to Invoke fetch files Based on Multi-Dir #8309

merged 22 commits into from
Nov 13, 2023

Conversation

honeyankit
Copy link
Contributor

@honeyankit honeyankit commented Oct 31, 2023 •
edited
Loading

Context

This pull request introduces enhancements to the FileFetcherCommand class, ensuring file fetching when dealing with repositories containing multiple directories.

Key Changes

Multi-Directory Support: The code now supports fetching files from multiple directories in a repository within same ecosystem

@honeyankit honeyankit self-assigned this Oct 31, 2023
@honeyankit
Copy link
Contributor Author

honeyankit commented Nov 2, 2023 •
edited
Loading

The updater test I added in python, since all the tests are in updater are in bundler so changing my test to Bundler for consistency.

@jakecoffman jakecoffman mentioned this pull request Nov 2, 2023
Merged
@honeyankit honeyankit marked this pull request as ready for review November 2, 2023 18:48
@honeyankit honeyankit requested a review from a team as a code owner November 2, 2023 18:48
@honeyankit honeyankit force-pushed the honeyankit/teach-updater-to-use-directories-for-multi-dir branch from fb72518 to 916886f Compare November 2, 2023 20:53
jakecoffman
jakecoffman approved these changes Nov 2, 2023
View reviewed changes
Copy link
Member

@jakecoffman jakecoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jakecoffman jakecoffman mentioned this pull request Nov 3, 2023
Merged
@honeyankit honeyankit force-pushed the honeyankit/teach-updater-to-use-directories-for-multi-dir branch from 00bd155 to 619d6e3 Compare November 4, 2023 16:53
deivid-rodriguez
deivid-rodriguez approved these changes Nov 6, 2023
View reviewed changes
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@honeyankit
adding modification to repo_contents to support multi-dir repo_contents
0b9595d
@honeyankit
Fixed merge conflicts
3b673b2
@honeyankit
Invoke dependency_files multiple times based on multi-dir and clean t…
7b5bfc9 
…he code
@honeyankit
removed debugger and more cleaning
9c36d6a
@honeyankit
add test cases for file_fetch to support multi-dir
46720c4
@honeyankit
fixed failing test for file_fetcher for multiple dir
b5f5236
@honeyankit
removed unwanted files and comments
5b6e0c1
@honeyankit
save correct contents of dependency_files when mutli-dir support is e…
07a1a98 
…nabled
@honeyankit
disable the lint warning and added comments related to GHES backward …
913d807 
…compatibility
@honeyankit
updater uses bundler tests so switchting from python to bundler
6f574ad
@honeyankit
added bundler fixute and removed python fixture
cd47b18
@honeyankit
added memoized to dependency_files_for_multi_directories since it get…
decbb60 
…ting called twice
@honeyankit
Fixed the test case with unused HTTP interactions left in the cassette
215787c
@honeyankit
added the retry for directroy specific loop
6edcf4d
@honeyankit honeyankit force-pushed the honeyankit/teach-updater-to-use-directories-for-multi-dir branch from 619d6e3 to 4d2238f Compare November 6, 2023 18:35
@jakecoffman
Copy link
Member

Two of the smoke tests (go-security, and go-update-pr) are now getting mode added which wasn't there before. I guess that isn't a problem, but just curious why that is.

@jakecoffman
Copy link
Member

Looking into the mode changes some more, on main it gets into this rescue because the go.mod is not found, but the go.sum is.

This appears to be because Dir.pwd is "/home/dependabot/dependabot-updater" for the go.mod fetch but "/home/dependabot/dependabot-updater/repo" for the go.sum fetch.

The new code must have reordered some things so that the go.mod is checked in the right pwd. But this seems like a bug in the Go FileFetcher.

I'm good with proceeding with this and fixing the tests as it seems more correct now!

@jakecoffman jakecoffman merged commit 747ab32 into main Nov 13, 2023
79 of 81 checks passed
@jakecoffman jakecoffman deleted the honeyankit/teach-updater-to-use-directories-for-multi-dir branch November 13, 2023 15:50
@jakecoffman jakecoffman mentioned this pull request Nov 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deivid-rodriguez deivid-rodriguez deivid-rodriguez approved these changes

@jakecoffman jakecoffman jakecoffman approved these changes

Assignees

@honeyankit honeyankit

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@honeyankit @jakecoffman @deivid-rodriguez