Graceful handling for duplicate package file names

[nathanwoulfe]

If I attempt to push a package where the file name already exists, umbpack throws an exception and my build breaks. In most cases that's probably the preferred behaviour, because it indicates that something isn't right in the build. However, I like to be difficult 😃

Example for my use case is the newly improved Nesting Contently build, where the build generates two packages - one being the NC datatype, and a second containing the optional property value converters. If I tag release 1.1.0 that has changes to the datatype but not the PVC package, the resulting build generates NestingContently.Umbraco_1.1.0.zip and NestingContently.Umbraco.ValueConverters_1.0.0.zip. Both are then pushed, but the latter fails because it is unchanged from the previous release, which throws an exception and breaks the build.

That's currently not a huge issues because the push task is the final step in the build, so it still completes but the build shows as failed. Ideally pushing a duplicate file name would return a warning, allowing the build to continue.

[ronaldbarendse]

UmbPack should indeed not throw exceptions on duplicate files/versions or at least add a flag to skip them, just like NuGet allows with the -SkipDuplicate flag (see CLI reference).

Just to be clear: ff pushing the package is skipped, it should also skip archiving other packages 👍

[jmayntzhusen]

Hey @nathanwoulfe

I think adding a flag to allow duplicate name uploads is a good idea, will obviously complicate our name-based archiving feature, but not too much.

Is this something you are up for contributing with? 🙂

The API we post packages to on Our allows duplicate package names, it is solely within UmbPack that it's disallowed.

Here is the point in the push command where it makes that check: https://github.com/umbraco/UmbPack/blob/dev/src/Verbs/PushCommand.cs#L79

[nathanwoulfe]

Hi @jmayntzhusen I can have a look, sure. Will shout if (when) I get lost!

[umbrabot]

Hi @nathanwoulfe,

We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post

Thanks muchly, from your friendly Umbraco GitHub bot :-)

[nathanwoulfe]

@jmayntzhusen what's the best way to test these changes?

[ronaldbarendse]

The changes are quite trivial:

• Add a SkipDuplicate flag to: https://github.com/umbraco/UmbPack/blob/2c8a9c9de7bdfa4859e2f01233071df15d860c9f/src/Verbs/PushCommand.cs#L24
• Add a new parameter/overload to pass in this flag and if enabled, switch to writing a warning/information message and exit with a 0 return code: https://github.com/umbraco/UmbPack/blob/0c0cebbcf63419f6620a9cf34b337bfa3e78f473/src/PackageHelper.cs#L103

@nathanwoulfe testing is probably easiest done with a test package on Our Umbraco, as I haven't seen a documented way to test this locally. But looking at the code, you'd most likely need to run a local instance of Our Umbraco: https://github.com/umbraco/UmbPack/blob/e86045ca5574ee3df783c5b4de90d99a43ca351c/src/Auth/AuthConstants.cs#L7-L11

[nathanwoulfe]

@ronaldbarendse that's essentially what I've done, except I'm returning a bool from the EnsurePackageDoesntAlreadyExist method. Keeps the method clean so that it does only what the name suggests, and avoids changing the signature (not that anyone will be consuming it outside umbPack)

I can use that bool as a guard for archiving and uploading too - if the package doesn't exist, we continue as normal. If it exists, I'm checking the flag then writing a message or throwing error as it does now.

[jmayntzhusen]

It is a @ronaldbarendse mentions, if you run a local version of Our and create a package + api key + build UmbPack in debug mode you can test that way. For a relatively simple change like this a draft package on Our is probably easier though.

We used to not have the check in early versions of UmbPack and back then you could successfully upload packages with the same name. The api on Our hasn't changed since then, so I am confident that it will "just" work, I would do a very quick test if you have a PR ready though.

[ronaldbarendse]

avoids changing the signature

Changing the return type also changes the signature 😉

The following code ensures it's backwards-compatible (but if that's not needed, first method can just be removed) and always exits when a duplicate package is found, so that logic isn't moved outside of this method:

[Obsolete("Use the overload specifying whether to skip duplicate packages instead.")]
public void EnsurePackageDoesntAlreadyExists(JArray packages, string packageFile)
    => EnsurePackageDoesntAlreadyExists(pacakges, packageFile, false);

public void EnsurePackageDoesntAlreadyExists(JArray packages, string packageFile, bool skipDuplicate)
{
    if (packages == null) return;

    var packageFileName = Path.GetFileName(packageFile);

    foreach(var package in packages)
    {
        var packageName = package.Value<string>("Name"); 
        if (packageName.Equals(packageFileName, StringComparison.InvariantCultureIgnoreCase))
        {
            if (skipDuplicate)
            {
                Console.WriteLine(Resources.Push_PackageSkipped, packageFileName);
                Environment.Exit(0);
            }
            else
            {
                WriteError(Resources.Push_PackageExists, packageFileName);
                Environment.Exit(80); // FILE_EXISTS
            }

        }
    }
}

UmbPack will also require an additional resource string for Resources.Push_PackageSkipped and maybe this should be written in orange/yellow, similar to how errors are written in red (although it should not write to the error output): https://github.com/umbraco/UmbPack/blob/0c0cebbcf63419f6620a9cf34b337bfa3e78f473/src/PackageHelper.cs#L167-L175

[jmayntzhusen]

UmbPack is not really supposed to be extensible, should just be installed as a tool. We don't care about breaking signatures as such, what is important is for the Our API to either still work out of the box or to be backwards compatible with prior umbpack versions.

In this case it's not an issue though, and in your example above we could simply do bool skipDuplicate = false, and remove the first method, right?

[nathanwoulfe]

I'll tidy up what I've been working on, and create a pr for discussion. I'm approaching it slightly differently but same end result.

[ronaldbarendse]

in your example above we could simply do bool skipDuplicate = false, and remove the first method, right?

Yes, in that case, the obsolete method can be removed and the new parameter added as optional indeed 👍

[nathanwoulfe]

I've added a PR for this, but then got to thinking - is there a reason why we wouldn't always skip duplicate names? The option really only changes the exitCode and message, it doesn't affect any other code execution (because it exits regardless of the option value).

[ronaldbarendse]

Exit codes are very important, so I think PR https://github.com/umbraco/UmbPack/pull/46 fixes this in the right way: exit with an error code if the flag is not present (current behavior), but exit normally when it is.

This way, the calling script can respond to the exit code and stop pushing other (related) packages or fail a build/deploy script, etc.

And if UmbPack would allow pushing multiple packages at the same time, this would make sure only the non-existing packages are pushed, instead of erroring out on the first duplicate one...