scolladon commented 11 months ago

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

[x] 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 commented 11 months ago

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

codecov[bot] commented 11 months ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

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

Additional details and impacted files

```diff @@ 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](https://app.codecov.io/gh/scolladon/sfdx-git-delta/pull/707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Sebastien) | Coverage Δ | | |---|---|---| | [src/utils/fsHelper.ts](https://app.codecov.io/gh/scolladon/sfdx-git-delta/pull/707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Sebastien#diff-c3JjL3V0aWxzL2ZzSGVscGVyLnRz) | `100.00% <100.00%> (ø)` | | | [src/utils/gitLfsHelper.ts](https://app.codecov.io/gh/scolladon/sfdx-git-delta/pull/707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Sebastien#diff-c3JjL3V0aWxzL2dpdExmc0hlbHBlci50cw==) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Russman12 commented 11 months ago

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:

ran git lfs track <some-file>
committed changes
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 commented 11 months ago

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 commented 11 months ago

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.

Russman12 commented 11 months ago

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 commented 11 months ago

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?

Russman12 commented 11 months ago

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

scolladon commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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.

codeclimate[bot] commented 11 months ago

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

View more on Code Climate.

github-actions[bot] commented 11 months ago

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!

scolladon / sfdx-git-delta

feat: support `git-lfs` #707

Explain your changes

Does this close any currently open issues?

Any particular element that can be tested locally

Any other comments

Codecov Report

Weekly Sync Minutes:

(a) pros and cons (commit)

(b) pros and cons (commit)

(Mix of a and b) pros and cons