Feature/with streams

[leboff]

@amtrack here's a quick and dirty POC I put together for performing the diff with streams. I think it's worth looking into because I'm seeing a significant improvement in memory usage and performance. Doing the diff like this will most likely solve #96

Here's an example of how to use:

 forceDev.Diff.stream(diff, {ignoreWhitespace: true}) //read stream for unified diff
        .pipe(forceDev.MetadataContainer.diffStream()) //diffs metadata creates manifests and filtered vinyls
        .pipe(forceDev.MetadataContainer.completeMetadataStream()) //completes metadata, roll up/filter manifests
        .pipe(forceDev.MetadataContainer.outputStream()); //vinyls that can be piped to dest

[amtrack]

That sounds great! Thanks! I’ll review this the next few days.

[amtrack]

Hi @leboff ,

please let me know if i can support you somehow.

It looks like the (linting) tests are not passing yet. Please run npm run test and fix the errors.

Today I created a simple integration test for the changeset command (https://github.com/amtrack/force-dev-tool/tree/test-changeset) which you can cherrypick/merge if you want. It's not final yet but maybe it can help you to get around manual testing all the time. Use npm run mocha -- -g changeset to quickly run only this test.

I will try to add some more meaningful test cases. Do you have any specific test cases in mind?

[leboff]

Thanks for taking a look @amtrack!

Were there already tests for the original changeset command? The streaming version should pass all of those to be considered valid.

I'll work on getting the lint issues fixed up and passing the mocha tests.

[amtrack]

Were there already tests for the original changeset command?

No, on the master branch there has only been a test for MetadataContainer so far.

I'll work on getting the lint issues fixed up and passing the mocha tests.

As far as i can see you already solved that. What else do you think is still left in order to complete this PR?

[leboff]

@amtrack still need to actually swap the CLI changeset command with the streaming version (or maybe make it an option) the test passes because I haven't modified anything from a CLI perspective yet, once I have the time I will do that though.

[leboff]

@amtrack I've added the streaming version to the CLI. Not entirely sure why the test that's failing is failing though. I seem to get the correct result when running from my command line

[amtrack]

@leboff OK, don't worry about the failing test force-dev-tool changeset should fail to create a changeset if there is no src/package.xml. It is part of my unfinished test suite.

I tried your PR on my system and noticed that the generated files appear to have lines joined that used to be in separate lines, e.g.

<?xml version="1.0" encoding="UTF-8"?>
<Profile xmlns="http://soap.sforce.com/2006/04/metadata">
    <fieldPermissions><editable>true</editable><field>Foo__c.Bar__c</field><readable>true</readable></fieldPermissions>
    <fieldPermissions><editable>true</editable><field>Foo__c.Baz__c</field><readable>true</readable></fieldPermissions>
    <fieldPermissions><editable>true</editable><field>Foo__c.Bazn__c</field><readable>true</readable></fieldPermissions>
</Profile>

should be formatted like this

<?xml version="1.0" encoding="UTF-8"?>
<Profile xmlns="http://soap.sforce.com/2006/04/metadata">
    <fieldPermissions>
        <editable>true</editable>
        <field>Foo__c.Bar__c</field>
        <readable>true</readable>
    </fieldPermissions>
    <fieldPermissions>
        <editable>true</editable>
        <field>Foo__c.Baz__c</field>
        <readable>true</readable>
    </fieldPermissions>
    <fieldPermissions>
        <editable>true</editable>
        <field>Foo__c.Bazn__c</field>
        <readable>true</readable>
    </fieldPermissions>
</Profile>

Can you reproduce this?

Besides that, this looks good and i'm looking forward to merge this PR once i have my test suite finished.

[leboff]

@amtrack That was actually semi-intentional. I forgot to include this as a cli option and had it hard coded, so the latest commit should fix that.

The reason I have it as an option is because the diff algorithm you have (metadata-file-container.js:196) that compares metadata for changes included whitespace unless i set preserveWhitespace to false (metadata-file-container.js:133) when it splits out child metadata.

For example

<fields>
    <fullName>Account_Description__c</fullName>
    <label>Account Description</label>
    <type>TextArea</type>
</fields>

Would be flagged as different from

<fields>
  <fullName>Account_Description__c</fullName>
  <label>Account Description</label>
  <type>TextArea</type>
</fields>

Which is not ideal for my use case where I have a lot of developers who may unintentionally produce differences like this.

Toggling off preserve whitespace however removes the formatting of the child lines, so you end up with those one-liners.

Salesforce deploys work fine without the pretty formatting, and since I believe the child metadata isn't parsed and content is just a string, we'd have to parse and then pretty print to get it to display correctly which didn't seem worth the CPU cycles (especially since this was mostly an exercise in optimization!)

[amtrack]

I see. Although Salesforce deploys work fine with that, personally i always prefer to be able to review my changes before the deployment and that can be done best when whitespace is preserved.

Does git diff --ignore-space-at-eol or --ignore-all-space help to reduce the amount of unintentional changes for you?

[leboff]

I don't think so, because for the diffs if I'm following correctly, you just use the file name / hashes to grab the files pre and post change with git show, so the differences will still be spotted in metadata-file-container. This assumes there are both whitespace/non-whitespace changes in the same file

[amtrack]

@leboff I finally added some unit and integration tests regarding changeset creation (#105) and those tests are passing. 🎉 There are still some test failures reported from CI which are all related to handling secret environment variables. I'll need to see how to improve that setup.

I will probably merge this PR the next days.

[leboff]

👍 thanks @amtrack looking forward to it!!

[amtrack]

Squashed and merged. Thank you very much @leboff !

[okram999]

Thanks guys @leboff @amtrack - i ran into this issue with ver 0.13.1. And i was debugging my CI server. Using the v1.1.0 seems to have done the trick. I also notice that the older version was taking around 6mins , while the new version reduces it to nearly 3mins