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

feat: support git-lfs #707

Merged
merged 8 commits into from
Oct 17, 2023
Merged

feat: support git-lfs #707

merged 8 commits into from
Oct 17, 2023

Conversation

scolladon
Copy link
Owner

@scolladon scolladon commented Oct 8, 2023

Explain your changes


Implement a way to find real content of git lfs files

No E2E test have been added yet, git-lfs seems to have different behavior depending the OS architecture.

Does this close any currently open issues?


closes #704

  • Jest tests added to cover the fix.
  • NUT tests added to cover the fix.
  • E2E tests added to cover the fix.

Any particular element that can be tested locally


Any other comments


@scolladon scolladon requested a review from mehdicherf as a code owner October 8, 2023 15:45
@scolladon
Copy link
Owner Author

Hi @Russman12 !

Could you help us by testing this PR locally with the repo with LFS files and tell us if it works please ?

You can follow those steps to install this PR locally and test it

@scolladon scolladon changed the title feat: implement git-lfs copy support feat: support git-lfs file copy Oct 8, 2023
@codecov
Copy link

codecov bot commented Oct 8, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (fd3cd8b) 100.00% compared to head (91bee3d) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #707   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           34        35    +1     
  Lines          974       988   +14     
  Branches        96        97    +1     
=========================================
+ Hits           974       988   +14     
Files Coverage Δ
src/utils/fsHelper.ts 100.00% <100.00%> (ø)
src/utils/gitLfsHelper.ts 100.00% <100.00%> (ø)

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

@scolladon scolladon changed the title feat: support git-lfs file copy feat: support git-lfs Oct 9, 2023
@scolladon scolladon force-pushed the feat/add-git-lfs-support branch from 798e76e to fe4a11f Compare October 9, 2023 11:03
@Russman12
Copy link
Contributor

Russman12 commented Oct 9, 2023

I have followed the steps mentioned in the CONTRIBUTING.md file and tested that non LFS files results in a delta directory creation as expected. One note is that I could not find a "test" branch, so I checked out the feature branch instead. After confirming it was working properly, I tested an LFS file by performing the following:

  1. ran git lfs track <some-file>
  2. committed changes
  3. executed sfdx sgd:source:delta --from "HEAD~1" --to "HEAD" -d

After the last command completes, the directory does not contain the LFS file at all, and the terminal is outputs the following error message:

TypeError: Cannot read properties of undefined (reading 'split')
    at getLFSObjectContentPath (...\sfdx-git-delta\src\utils\gitLfsHelper.ts:13:36)
    at readPathFromGitAsBuffer (...\sfdx-git-delta\src\utils\fsHelper.ts:64:44)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async copyFiles (...\sfdx-git-delta\src\utils\fsHelper.ts:28:32)
    at async ResourceHandler._copy (...\sfdx-git-delta\src\service\standardHandler.ts:143:7)
    at async ResourceHandler._copyWithMetaFile (...\sfdx-git-delta\src\service\standardHandler.ts:131:7)
    at async ResourceHandler.handleAddition (...\sfdx-git-delta\src\service\standardHandler.ts:92:5)
    at async ResourceHandler.handleAddition (...\sfdx-git-delta\src\service\inResourceHandler.ts:25:5)
    at async ResourceHandler.handleModification (...\sfdx-git-delta\src\service\standardHandler.ts:100:5)
    at async ResourceHandler.handle (...\sfdx-git-delta\src\service\standardHandler.ts:76:13)

I also tested creating a new file that was LFS tracked upon creation. In that scenario, I am getting the previous error as well as the following:

Error: fatal: path 'force-app/main/default/staticresources/AccountOwnerAssignmentRule2.resource' does not exist in 'HEAD~4'

    at ChildProcess.<anonymous> (...\sfdx-git-delta\src\utils\childProcessUtils.ts:52:16)
    at ChildProcess.emit (node:events:513:28)
    at ChildProcess.emit (node:domain:489:12)
    at maybeClose (node:internal/child_process:1098:16)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:304:5)
Error: fatal: path 'force-app/main/default/staticresources/AccountOwnerAssignmentRule2.resource-meta.resource-meta.xml' does not exist in 'HEAD~4'

    at ChildProcess.<anonymous> (...\sfdx-git-delta\src\utils\childProcessUtils.ts:52:16)
    at ChildProcess.emit (node:events:513:28)
    at ChildProcess.emit (node:domain:489:12)
    at maybeClose (node:internal/child_process:1098:16)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:304:5)

@scolladon
Copy link
Owner Author

scolladon commented Oct 10, 2023

Hi @Russman12, thanks for testing.

What is your environment please ? (os, node version, git version, lfs version, yarn version, sfdx version)

And can you send us the AccountOwnerAssignmentRule2.resource file content please (the lfs pointer one)

Also, could you retry with the latest version of this PR please ? Let us know the result please

@scolladon scolladon force-pushed the feat/add-git-lfs-support branch from f939723 to f463307 Compare October 10, 2023 07:52
@scolladon
Copy link
Owner Author

scolladon commented Oct 10, 2023

Weekly Sync Minutes:

Compare (a) copying file efficiently without storing the content in memory
vs (b) fetching real content each time we want to access a LFS file.

context: Metadata deployment
Limitations: 10000 files, 400mb uncompressed size and 39mb compressed size

(a) pros and cons (commit)

  • +copy very fast and efficiently
  • +does not impact RAM
  • -does not work with inFile metadata as they will not be resolved when introspected

(b) pros and cons (commit)

  • +work with inFile metadata as they will be resolved when introspected
  • -huge impact on the memory as every LFS will be loaded inside a Buffer variable

(Mix of a and b) pros and cons

  • +work with inFile metadata as they will be resolved when introspected
  • +copy very fast and efficiently
  • +does not impact RAM when copying
  • -huge impact on the memory when LFS inFile metadata are introspected

For now we think we can go with option (b).
This is the design that support the most features and is the easiest to implement.
It would not work with very large file, but as the max deployable uncompress metadata is 400mb it can fit in the memory in most of the CIs used nowadays.

@scolladon scolladon force-pushed the feat/add-git-lfs-support branch from f463307 to eed449f Compare October 10, 2023 10:00
@Russman12
Copy link
Contributor

Hi @Russman12, thanks for testing.

What is your environment please ? (os, node version, git version, lfs version, yarn version, sfdx version)

And can you send us the AccountOwnerAssignmentRule2.resource file content please (the lfs pointer one)

Also, could you retry with the latest version of this PR please ? Let us know the result please

After pulling the latest version of this feature branch repackaging/relinking the commands, I was able to get it working when making existing files LFS tracked. However, when creating a new file and adding it to LFS at the same time, it still gives me an error.

Below are the commands I ran to reproduce the new file scenario:

git lfs track force-app/main/default/staticresources/my-file.txt
echo "this is a large file" > force-app/main/default/staticresources/my-file.txt
echo "<?xml version="1.0" encoding="UTF-8"?>
<StaticResource xmlns="http://soap.sforce.com/2006/04/metadata">
    <cacheControl>Private</cacheControl>
    <contentType>text/plain</contentType>
</StaticResource>" > force-app/main/default/staticresources/my-file.resource-meta.xml
git add . && git commit -m "added my-file.txt"
sfdx sgd:source:delta --from "HEAD~1" --to "HEAD" -d

This is the output after the last command completes:

Error: fatal: path 'force-app/main/default/staticresources/my-file.resource' does not exist in 'HEAD'

    at ChildProcess.<anonymous> (...\sfdx-git-delta\src\utils\childProcessUtils.ts:52:16)
    at ChildProcess.emit (node:events:513:28)
    at ChildProcess.emit (node:domain:489:12)
    at maybeClose (node:internal/child_process:1098:16)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:304:5)
Error: fatal: path 'force-app/main/default/staticresources/my-file.resource-meta.resource-meta.xml' does not exist in 'HEAD'

    at ChildProcess.<anonymous> (...\sfdx-git-delta\src\utils\childProcessUtils.ts:52:16)
    at ChildProcess.emit (node:events:513:28)
    at ChildProcess.emit (node:domain:489:12)
    at maybeClose (node:internal/child_process:1098:16)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:304:5)

Despite this error message, the output directory contains all the expected files and subdirectories (my-file.resource-meta.xml, my-file.txt, and package.xml) and the content of each file is correct as well. Based on the error message, I think something may be appending a redundant "resource-meta" to the .xml file.

Windows 10 v10.0.19044.3448
I used both cmd and git bash for windows with the same result.
node v19.0.0
git 2.42.0.windows.2
git-lfs 3.4.0
yarn 1.22.19
sfdx @salesforce/cli/2.10.2

Hope this helps!

@scolladon
Copy link
Owner Author

Thanks for this feedback @Russman12
I have updated the PR to fix the error message display.
The implementation seems ok on my end.
Could you test one more time and let us know the result on your end please?

@scolladon scolladon force-pushed the feat/add-git-lfs-support branch from 10b4bb3 to 4de6f08 Compare October 11, 2023 08:14
@Russman12
Copy link
Contributor

Thanks for the quick resolution @scolladon! Yes, this has fixed all the issues I encountered. Great work!

@scolladon
Copy link
Owner Author

Thank you very much for your help here @Russman12, great contribution.

One more thing, what do you think about the design choice we made here ? Do you agree with it, considering the metadata API limitation ?

@Russman12
Copy link
Contributor

Russman12 commented Oct 11, 2023

Thank you very much for your help here @Russman12, great contribution.

One more thing, what do you think about the design choice we made here ? Do you agree with it, considering the metadata API limitation ?

Forgive me, but I don't quite understand what you mean by "does not work with inFile metadata as they will not be resolved when introspected". Are you saying that for option "A" if a "meta.xml" file is tracked as LFS, that won't build the package properly since the content can't be read?

@scolladon
Copy link
Owner Author

It means for metadata contained in file (like per exemple CustomLabels). it won't be able to detect the actual changes inside the file because option (a) does not get the content.
It does not populate package.xml with changes from inside the file and it does not scope the file to just what has changed.
It just copies the file when needed.

@Russman12
Copy link
Contributor

Oh, I see. Good catch! Yeah, I mean unless you were to write something that treats files like that different, than I think option B is the better solution.

@scolladon scolladon force-pushed the feat/add-git-lfs-support branch from 4de6f08 to bb05f30 Compare October 17, 2023 12:22
mehdicherf
mehdicherf previously approved these changes Oct 17, 2023
@codeclimate
Copy link

codeclimate bot commented Oct 17, 2023

Code Climate has analyzed commit 91bee3d and detected 0 issues on this pull request.

View more on Code Climate.

@scolladon scolladon merged commit b27abc8 into main Oct 17, 2023
20 of 22 checks passed
@scolladon scolladon deleted the feat/add-git-lfs-support branch October 17, 2023 12:49
@github-actions
Copy link

Shipped in release v5.27.0.
You can install the new version using the version number or the latest-rc channel

$ sfdx plugins:install sfdx-git-delta@latest-rc
$ sfdx plugins:install sfdx-git-delta@v5.27.0

Happy incremental deployment!

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.

Add Git LFS Support
3 participants