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.

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

Update fetch.go to include a Depth specifier #1732

Conversation

ollie-kane-CB
Copy link
Contributor

For repositories with large histories, since there doesn't appear to be caching of the git repository, we shouldn't pull all history as it's not needed for the purpose of building. This one line change significantly improves build performance when irrelevant git history is large.

For repositories with large histories, since there doesn't appear to be caching of the git repository, we shouldn't pull all history as it's not needed for the purpose of building.  This one line change significantly improves build performance when irrelevant git history is large. 

Signed-off-by: ollie-kane-CB <[email protected]>
@ollie-kane-CB ollie-kane-CB requested a review from a team as a code owner October 14, 2024 16:46
@ollie-kane-CB
Copy link
Contributor Author

I see the tests have failed. I'm a bit unfamiliar with Go, but if the effort is needed, i can attempt to figure out what's wrong. I expected this to be the equiliv of adding the --depth 1 flag to a git clone

git clone --depth 1 REPO_URL .

rather than

git clone REPO_URL .

@ollie-kane-CB
Copy link
Contributor Author

After reading on the web some, it seems like this link may have something to do with it.

rancher/rancher#42659

They suggest that in addition to the Depth, one must also specify the Force param, otherwise it won't create the local ref. I'll attempt this and see if it passes the test suite. If not, rather than generate a lot of noise, I'll see if I can determine how to run your test suite myself.

Based on rancher/rancher#42659, it appears that it may be necessary to include a force flag

Signed-off-by: ollie-kane-CB <[email protected]>
@ollie-kane-CB ollie-kane-CB marked this pull request as draft October 14, 2024 23:05
@ollie-kane-CB
Copy link
Contributor Author

Modified this to Draft. I figured out how to build and run the project locally. Will iterate and return.

Signed-off-by: ollie-kane-CB <[email protected]>
Signed-off-by: ollie-kane-CB <[email protected]>
@ollie-kane-CB ollie-kane-CB marked this pull request as ready for review October 15, 2024 16:36
@ollie-kane-CB
Copy link
Contributor Author

Updated this to pass the unit tests. Ended up being slightly more complicated than I expected.

In order to support the Depth: 1, the Hash resolver process needed to be updated. Instead of resolving the gitRevision using the results of a Fetch --all, instead use the GitRemove.List operation to get a list of refs only, and from those resolve the hash. Once the hash is known, use that in the FetchOptions to configure only fetching of the data required.

A small side effect is the 'doesnotexist' test needed to have it's error message updated as the error thrown when there's no match is different now.

Sorry for the delay and noise here!

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 69.82%. Comparing base (49014e2) to head (f8b073d).
Report is 51 commits behind head on main.

Files with missing lines Patch % Lines
pkg/git/fetch.go 86.66% 1 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1732      +/-   ##
==========================================
+ Coverage   67.34%   69.82%   +2.48%     
==========================================
  Files         140      144       +4     
  Lines        8886     7556    -1330     
==========================================
- Hits         5984     5276     -708     
+ Misses       2393     1760     -633     
- Partials      509      520      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@tomkennedy513 tomkennedy513 left a comment

Choose a reason for hiding this comment

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

lgtm

@tomkennedy513 tomkennedy513 merged commit c8423ab into buildpacks-community:main Oct 15, 2024
1 check passed
@ollie-kane-CB
Copy link
Contributor Author

lgtm

Thanks for the merge!

I'm looking into adopting this project into my stack. I don't know if I'll ever get around to it, but if you were to pick a feature/bug/issue that'd be the best value-add, what would that be? If I'm swimming in this lane, it's possible that a weekend here i may take a stab at the backlog.

@tomkennedy513
Copy link
Collaborator

lgtm

Thanks for the merge!

I'm looking into adopting this project into my stack. I don't know if I'll ever get around to it, but if you were to pick a feature/bug/issue that'd be the best value-add, what would that be? If I'm swimming in this lane, it's possible that a weekend here i may take a stab at the backlog.

hmm, I'm not sure I have a particular issue I would recommend, but I would say that playing around with kpack and seeing what doesn't work right or what annoys you that could be improved might be a good way to start

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants