-
Notifications
You must be signed in to change notification settings - Fork 187
Use sparse checkout for components #1701
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
Use sparse checkout for components #1701
Conversation
|
using these refactoring won't be easy, we have a couple of integration test, make sure those passes to prevent regression bugs |
|
I worked on the next version to. Changes;
The tests are working on my end and I can use it with our private Gitlab instance. |
|
nice work man, thanks for the contribution. in general lgtm, but will take another look when i have more spare time. I see you have TODOs in your pr description.
re. no.1 i think you have resolved it right? On a seperate topic, i also see that you've considered using git archive. a while back, i did some profiling and found that git archive can be more performant on elephant size repo like gitlab.com interestingly, i face different error from you when attempting git archive via https. |
|
I did a last round of refactoring and removed
Yes, now the result is cached
That TODO was mostly for myself. I wasn't sure what some code did, but now I know and refactored the code accordingly.
Done :) I will update the opening post Yeah, it's a shame
Super interesting profiling data, thank you for showing me that. |
src/parser-includes.ts
Outdated
| `git sparse-checkout set --no-cone ${files[0]} ${files[1]}`, | ||
| "git checkout", | ||
| `cd ${cwd}/${stateDir}`, | ||
| `rsync -a --ignore-missing-args ${cwd}/${target}.${ext}/templates ${cwd}/${target}`, // use rsnyc to ignore missing templates/, the check for existence is in a later step |
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.
i see that this is inspired from downloadIncludeProjectFile , wondering if we should clean up the temporary folder (${cwd}/${target}.${ext})
Not sure if i am missing something. I dont think --ignore-missing-args is required right ? since the templates folder should always exist
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.
IMHO, we should always cleanup in both downloadIncludeProjectFile and downloadIncludeComponent
The /template directory won't be created by the checkout if neither templates/component.yml nor templates/component/template.yml exists. This was a measure to defer the error handling for missing files to a later time.
We can either do the check later (as is done currently) or duplicate the check here. I'm open to anything.
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.
if you don't mind, could you update to perform the clean up .
other than that i think we're good to go ! thanks, nice work
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.
Sure, I added the cleanup code to both functions
Thank you for your review!
|
Sorry forgot to run the linter. Im not using TS often. |
This PR reworks how components are pulled from the remote. Which would fail, if you tried to pull files from a private project with http(s)
Old way
New way
Additional changes
I had to change how
parseIncludeComponentfetches to the tags. From try/catch to if/else, because if a firewall blocks ssh it takes a few minutes before the call timeouts...TODOs
I'm callingparseIncludeComponenttwice, which is expensive, due to fetching tagsDone, the result is cached now
I'm not sure if the inner expands done for projects are neccessary for componentsDone, code is refactored so it can be used for project and component includes
This PR remove the only use ofUtils.remoteFileExist, but I don't know if you want to keep it aroundDone, function is removed