umbraco / UmbPack

13 stars 13 forks source link

Add replaceable properties to the pack command #38

Closed mattbrailsford closed 4 years ago

mattbrailsford commented 4 years ago

This PR fixes #35 by adding a new -p option that accepts a series of name value pairs where the name defines a key that should be replaced in the package.xml file and the value defines the replacement value.

umbpack pack path/to/package.xml -p MyProp1=Val1;MyProp2=Val2

With the above command, any text tokens for $MyProp1$ and $MyProp2$ in the package.xml will be replaces with Val1 and Val2 values accordingly.

mattbrailsford commented 4 years ago

At the moment this loads the whole package manifest into memory and does a find and replace, which if it's a minimal package.xml file then that'll be fine, but if people are defining docs types etc, then this could get pretty big. This said, I don't know how many people are using the doc types / data types etc feature as this isn't something that works across Umbraco and NuGet so I suspect many devs will be using migrations to perform those tasks instead.

jmayntzhusen commented 4 years ago

Yo @mattbrailsford This is a great idea! It only loads it into memory if you specify the -p option, right?

So if it's only the starter kits who'd really be hit by this, and then only if they use this option then I think it's ok 🙂

We have some updates coming to the package format RFC soon where a suggested first step will be to "tidy up" the structure of the current format to be more similar to NuGet - fx to create a content folder and then have each doctype, etc added as separate files. Will give a much nicer overview and bring it more inline with how NuGet are doing it. With the added benefit of keeping the package.xml file as mostly meta data and would solve this potential memory issue 🙂 More on that in the RFC once it has been completed 😉

Will get to reviewing and testing this as soon as we can - holidays may increase our response times a bit these days..

mattbrailsford commented 4 years ago

@jmayntzhusen it currently loads in to memory regardless but I can easily fix this so that it only does it if you provide a -p flag for sure. I'll submit an update in a second.

The idea of moving doc types to disc sounds great and make things a lot more consistent and negate this problem altogether so I'm all for it. 👍

mattbrailsford commented 4 years ago

Ok, I've updated it so that we don't load the XML into memory if there aren't any properties to replacement, however, I don't think this is actually saving anything now I look at it as XElement.Load will still load the whole file into memory so 🤷‍♂️

Either way it's been updated so can't hurt.

jmayntzhusen commented 4 years ago

Tested the following cases:

Works perfectly! So sorry for how long it's been without review Matt!

This makes me want to remove the -v and -n pack options as they could "just" be replaceable properties, but as they are a lot more common than the others it may be better to keep them..

jmayntzhusen commented 4 years ago

This will be part of an upcoming 0.9.6 release which we aim to get out at the latest next week 🙂

mattbrailsford commented 4 years ago

Hey @jmayntzhusen no problem at all

Like you say, I think -v and -n still have value so worth keeping, but it's another option at least 👍