adds flag to skip duplicate filenames, adds writewarning function,

[nathanwoulfe]

I was overthinking this - just needs to response to the option value when checking the package exists, then either exit with an appropriate exitCode if required. Because we exit whenever a duplicate exists, there's no issue with archiving or subsequent code.

I've also shifted the archive package logic in PushCommand.Main into a separate method (similar to UploadPackage), to reduce responsibilities in the Main method.

I've tested this with a local run against a draft package - if I attempt to push without the -s flag, umbpack exits with an error, if i push with the -s flag, umbpack exits with the warning message.

[ronaldbarendse]

Looks good to me, although you might want to fix the indentation and either complete the XML documentation or remove the empty tags 👍

[nathanwoulfe]

@ronaldbarendse my bad, didn't notice the messed up indentation. Fixed now though.

[jmayntzhusen]

Hey Nathan!

When testing this it didn't work, primarily because the if/else condition here exited the program and gave a warning no matter what you set it to 🙂 https://github.com/umbraco/UmbPack/pull/46/commits/7273fad20c15f896cb0d9b54a4bdc510e06b9a09#diff-0b3c49d26e061927a49740015359294aa24da7e07eb9acaef8eef6eec0f3b002R114

I've suggested some changes via https://github.com/umbraco/UmbPack/pull/46/commits/fd0b64efc51ad52b1870618528121145f1fd4d53

I've renamed skipduplicates to allowduplicates. When running and testing with a local version of Our it now works for me:

Please do challenge me on the help texts, etc if you have any ideas or concerns!

[ronaldbarendse]

@jmayntzhusen Releasing duplicate package names will only cause confusion and these changes won't solve the issue described in https://github.com/umbraco/UmbPack/issues/44. The problem is that UmbPack currently returns with an exit code if a duplicate package name is pushed, but that makes build scripts fail when it should actually just skip pushing that package (without raising an error).

[ronaldbarendse]

When testing this it didn't work, primarily because the if/else condition here exited the program and gave a warning no matter what you set it to

It currently always exits, because only a single package can be pushed and this should always be unique, but the exit code differs: without the -SkipDuplicates flag a message is written to the error output and the exit code is 80, otherwise it only writes a warning to the standard output and exits cleanly with exit code 0.

[jmayntzhusen]

Oh I completely misunderstood what it was you were trying to do here! 🙈

[jmayntzhusen]

Alright - reverted my code and retested!

If -s true is set:

If -s is omitted:

Unsure on how to tell whether it would fail a CI/CD run fx.

[ronaldbarendse]

Hmm, looks like it always returns exit code 0, so that would mean all Environment.Exit(...) calls don't set it correctly:

https://github.com/umbraco/UmbPack/blob/087f8d7808273edfbb398a6e08db6c2330d2276c/src/PackageHelper.cs#L121-L122

This might be because the PushCommand always returns 0 (and exiting before returning doesn't set it): https://github.com/umbraco/UmbPack/blob/087f8d7808273edfbb398a6e08db6c2330d2276c/src/Verbs/PushCommand.cs#L95

[nathanwoulfe]

@ronaldbarendse this would have been the case prior to these changes though, wouldn't it? Previously would have hit Environment.Exit(80) then returned 0?

[ronaldbarendse]

This would have been the case prior to these changes though, wouldn't it? Previously would have hit Environment.Exit(80) then returned 0?

That seems to be the case, yes! And as the commands (like PushCommand) require a return value for the exit code, using Environment.Exit(80) seems like a hack anyway.

Most of the PackageHelper methods would need a rewrite for this though, e.g. return a nullable integer as optional exit code.

public int? EnsurePackageDoesntAlreadyExists(JArray packages, string packageFile, bool skipDuplicates = false)
{
    if (packages == null) return null;

    var packageFileName = Path.GetFileName(packageFile);

    foreach (var package in packages)
    {
        var packageName = package.Value<string>("Name");
        if (packageName.Equals(packageFileName, StringComparison.InvariantCultureIgnoreCase))
        {
            if (skipDuplicates)
            {
                WriteWarning(Resources.Push_PackageSkipped, packageFileName);
                return 0;
            }
            else
            {
                WriteError(Resources.Push_PackageExists, packageFileName);
                return 80;
            }
        }
    }

    return null;
}

Which is then used as:

if (packageHelper.EnsurePackageDoesntAlreadyExists(packages, filePath, options.SkipDuplicates) is int exitCode)
{
    return exitCode;
}