Make options typed, and include data type info in json help output (#65)

Instead of just assuming all our command line options are strings, this changes our options parsing to use generics, and uses the built in option parsing.

This means:
* we dont need to do safe parsing (which we were doing inconsistently anyway - quite a few commands failed if you passed invalid data)
* we parse enums properly (one of our tests was using dodgy data and we didn't know
* we now flag sensitive options
* our (json formatted) help output now includes data types and enum values

source/Octo.Tests/Commands/CompleteCommandFixture.cs

@@ -124,8 +124,8 @@ public class TestCommand : CommandBase
         public TestCommand(ICommandOutputProvider commandOutputProvider) : base(commandOutputProvider)
         {
             var options = Options.For("Test group");
-            options.Add("apiKey", "api key", v => ApiKey = v);
-            options.Add("url", "url", v => Url = v);
+            options.Add<string>("apiKey", "api key", v => ApiKey = v);
+            options.Add<string>("url", "url", v => Url = v);
         }
 
         public string Url { get; set; }

source/Octo.Tests/Commands/DummyApiCommand.cs

@@ -14,7 +14,7 @@ public DummyApiCommand(IOctopusAsyncRepositoryFactory repositoryFactory, IOctopu
             : base(clientFactory, repositoryFactory, fileSystem, commandOutputProvider)
         {
             var options = Options.For("Dummy");
-            options.Add("pill=", "Red or Blue. Blue, the story ends. Red, stay in Wonderland and see how deep the rabbit hole goes.", v => pill = v);
+            options.Add<string>("pill=", "Red or Blue. Blue, the story ends. Red, stay in Wonderland and see how deep the rabbit hole goes.", v => pill = v);
             commandOutputProvider.Debug("Pill: " + pill);
         }
 

source/Octo.Tests/Commands/ListMachinesCommandFixture.cs

@@ -315,7 +315,7 @@ public async Task ShouldSupportStateFilters()
         [Test]
         public async Task ShouldThrowIfUsingHealthStatusPre34()
         {
-            CommandLineArgs.Add("--health-status=Online");
+            CommandLineArgs.Add("--health-status=Healthy");
             (await Repository.LoadRootDocument()).Version = "3.1.0";
 
             Func<Task> exec = () => listMachinesCommand.Execute(CommandLineArgs.ToArray());

source/Octo.Tests/Commands/OptionsFixture.cs

@@ -1,4 +1,6 @@
-using NUnit.Framework;
+using System.Security.Cryptography.X509Certificates;
+using NUnit.Framework;
+using Octopus.Cli.Commands.Package;
 using Octopus.Cli.Infrastructure;
 
 namespace Octo.Tests.Commands
@@ -16,17 +18,25 @@ public void ShouldBeCaseInsensitive(string parameter1, string parameter2)
         {
             var apiKey = string.Empty;
             var foo = string.Empty;
-
-            var optionSet = new OptionSet()
-            {
-                {"apiKey=", "API key", v => apiKey = v},
-                {"foo=", "Foo", v => foo = v}
-            };
+
+            var optionSet = new OptionSet();
+            optionSet.Add<string>("apiKey=", "API key", v => apiKey = v);
+            optionSet.Add<string>("foo=", "Foo", v => foo = v);
 
             optionSet.Parse(new[] {parameter1, parameter2});
 
             Assert.That(apiKey, Is.EqualTo("abc123"));
             Assert.That(foo, Is.EqualTo("bar"));
         }
+
+        [Test]
+        public void ShowsValidValuesForEnums()
+        {
+            var optionSet = new OptionSet();
+            optionSet.Add<PackageFormat>("packageFormat=", "Package Format", v => { });
+
+            var ex = Assert.Throws<CommandException>(() => optionSet.Parse(new[] {"--packageFormat", "invalidvalue"}));
+            Assert.That(ex.Message, Is.EqualTo("Could not convert string `invalidvalue' to type PackageFormat for option `--packageFormat'. Valid values are Zip, Nupkg and Nuget."));
+        }
     }
 }

source/Octo.Tests/Commands/SpeakCommand.cs

@@ -16,7 +16,7 @@ public override Task Execute(string[] commandLineArguments)
         public SpeakCommand(ICommandOutputProvider commandOutputProvider) : base(commandOutputProvider)
         {
             var options = Options.For("default");
-            options.Add("message=", m => { });
+            options.Add<string>("message=", m => { });
         }
     }
 }

source/Octo.Tests/Commands/SupportFormattedOutputFixture.cs

@@ -4,6 +4,7 @@
 using FluentAssertions;
 using Newtonsoft.Json;
 using NUnit.Framework;
+using Octopus.Cli.Infrastructure;
 
 namespace Octo.Tests.Commands
 {
@@ -35,15 +36,16 @@ public async Task FormattedOutput_FormatSetToJson()
         }
 
         [Test]
-        public async Task FormattedOutput_FormatInvalid()
+        public void FormattedOutput_FormatInvalid()
         {
             var command = new DummyApiCommandWithFormattedOutputSupport(ClientFactory, RepositoryFactory, FileSystem, CommandOutputProvider);
             CommandLineArgs.Add("--helpOutputFormat=blah");
 
-            await command.Execute(CommandLineArgs.ToArray()).ConfigureAwait(false);
+            var exception = Assert.ThrowsAsync<CommandException>(async () => await command.Execute(CommandLineArgs.ToArray()).ConfigureAwait(false));
 
-            command.PrintJsonOutputCalled.ShouldBeEquivalentTo(false);
-            command.PrintDefaultOutputCalled.ShouldBeEquivalentTo(true);
+            command.PrintJsonOutputCalled.Should().BeFalse();
+            command.PrintDefaultOutputCalled.Should().BeFalse();
+            exception.Message.Should().Be("Could not convert string `blah' to type OutputFormat for option `--helpOutputFormat'. Valid values are Default and Json.");
         }
 
         [Test]

source/Octopus.Cli/Commands/ApiCommand.cs

@@ -59,16 +59,16 @@ protected ApiCommand(IOctopusClientFactory clientFactory, IOctopusAsyncRepositor
             this.FileSystem = fileSystem;
 
             var options = Options.For("Common options");
-            options.Add("server=", $"[Optional] The base URL for your Octopus Server, e.g., 'https://octopus.example.com/'. This URL can also be set in the {ServerUrlEnvVar} environment variable.", v => serverBaseUrl = v);
-            options.Add("apiKey=", $"[Optional] Your API key. Get this from the user profile page. Your must provide an apiKey or username and password. If the guest account is enabled, a key of API-GUEST can be used. This key can also be set in the {ApiKeyEnvVar} environment variable.", v => apiKey = v);
-            options.Add("user=", $"[Optional] Username to use when authenticating with the server. Your must provide an apiKey or username and password. This Username can also be set in the {UsernameEnvVar} environment variable.", v => username = v);
-            options.Add("pass=", $"[Optional] Password to use when authenticating with the server. This Password can also be set in the {PasswordEnvVar} environment variable.", v => password = v);
-
-            options.Add("configFile=", "[Optional] Text file of default values, with one 'key = value' per line.", v => ReadAdditionalInputsFromConfigurationFile(v));
-            options.Add("debug", "[Optional] Enable debug logging", v => enableDebugging = true);
-            options.Add("ignoreSslErrors", "[Optional] Set this flag if your Octopus Server uses HTTPS but the certificate is not trusted on this machine. Any certificate errors will be ignored. WARNING: this option may create a security vulnerability.", v => ignoreSslErrors = true);
-            options.Add("enableServiceMessages", "[Optional] Enable TeamCity or Team Foundation Build service messages when logging.", v => commandOutputProvider.EnableServiceMessages());
-            options.Add("timeout=", $"[Optional] Timeout in seconds for network operations. Default is {ApiConstants.DefaultClientRequestTimeout/1000}.", v =>
+            options.Add<string>("server=", $"[Optional] The base URL for your Octopus Server, e.g., 'https://octopus.example.com/'. This URL can also be set in the {ServerUrlEnvVar} environment variable.", v => serverBaseUrl = v);
+            options.Add<string>("apiKey=", $"[Optional] Your API key. Get this from the user profile page. Your must provide an apiKey or username and password. If the guest account is enabled, a key of API-GUEST can be used. This key can also be set in the {ApiKeyEnvVar} environment variable.", v => apiKey = v, sensitive: true);
+            options.Add<string>("user=", $"[Optional] Username to use when authenticating with the server. Your must provide an apiKey or username and password. This Username can also be set in the {UsernameEnvVar} environment variable.", v => username = v);
+            options.Add<string>("pass=", $"[Optional] Password to use when authenticating with the server. This Password can also be set in the {PasswordEnvVar} environment variable.", v => password = v, sensitive: true);
+
+            options.Add<string>("configFile=", "[Optional] Text file of default values, with one 'key = value' per line.", v => ReadAdditionalInputsFromConfigurationFile(v));
+            options.Add<bool>("debug", "[Optional] Enable debug logging", v => enableDebugging = true);
+            options.Add<bool>("ignoreSslErrors", "[Optional] Set this flag if your Octopus Server uses HTTPS but the certificate is not trusted on this machine. Any certificate errors will be ignored. WARNING: this option may create a security vulnerability.", v => ignoreSslErrors = true);
+            options.Add<bool>("enableServiceMessages", "[Optional] Enable TeamCity or Team Foundation Build service messages when logging.", v => commandOutputProvider.EnableServiceMessages());
+            options.Add<string>("timeout=", $"[Optional] Timeout in seconds for network operations. Default is {ApiConstants.DefaultClientRequestTimeout/1000}.", v =>
             {
                 if (int.TryParse(v, out var parsedInt))
                     clientOptions.Timeout = TimeSpan.FromSeconds(parsedInt);
@@ -77,18 +77,12 @@ protected ApiCommand(IOctopusClientFactory clientFactory, IOctopusAsyncRepositor
                 else
                     throw new CommandException($"Unable to parse '{v}' as a timespan or an integer.");
             });
-            options.Add("proxy=", $"[Optional] The URL of the proxy to use, e.g., 'https://proxy.example.com'.", v => clientOptions.Proxy = v);
-            options.Add("proxyUser=", $"[Optional] The username for the proxy.", v => clientOptions.ProxyUsername = v);
-            options.Add("proxyPass=", $"[Optional] The password for the proxy. If both the username and password are omitted and proxyAddress is specified, the default credentials are used. ", v => clientOptions.ProxyPassword = v);
-            options.Add("space=", $"[Optional] The name or ID of a space within which this command will be executed. The default space will be used if it is omitted. ", v => spaceNameOrId = v);
+            options.Add<string>("proxy=", $"[Optional] The URL of the proxy to use, e.g., 'https://proxy.example.com'.", v => clientOptions.Proxy = v);
+            options.Add<string>("proxyUser=", $"[Optional] The username for the proxy.", v => clientOptions.ProxyUsername = v);
+            options.Add<string>("proxyPass=", $"[Optional] The password for the proxy. If both the username and password are omitted and proxyAddress is specified, the default credentials are used.", v => clientOptions.ProxyPassword = v, sensitive: true);
+            options.Add<string>("space=", $"[Optional] The name or ID of a space within which this command will be executed. The default space will be used if it is omitted.", v => spaceNameOrId = v);
 #if NETFRAMEWORK
-            options.Add("keepalive=", "[Optional] How frequently (in seconds) to send a TCP keepalive packet.", input =>
-            {
-                if (int.TryParse(input, out var parsedInt))
-                    keepAlive = parsedInt * 1000;
-                else
-                    throw new CommandException($"Unable to parse '{input}' as an integer.");
-            });
+            options.Add<int>("keepalive=", "[Optional] How frequently (in seconds) to send a TCP keepalive packet.", input => keepAlive = input * 1000);
 #endif
             options.AddLogLevelOptions();
         }
@@ -275,32 +269,6 @@ private void Respond()
             }
         }
 
-        static NetworkCredential ParseCredentials(string username, string password)
-        {
-            if (string.IsNullOrWhiteSpace(username))
-                return CredentialCache.DefaultNetworkCredentials;
-
-            var split = username.Split('\\');
-            if (split.Length == 2)
-            {
-                var domain = split.First();
-                username = split.Last();
-
-                return new NetworkCredential(username, password, domain);
-            }
-
-            return new NetworkCredential(username, password);
-        }
-
-        protected void SetFlagState(string input, ref bool? setter)
-        {
-            bool tempBool;
-            if (bool.TryParse(input, out tempBool))
-            {
-                setter = tempBool;
-            }
-        }
-
         protected List<string> ReadAdditionalInputsFromConfigurationFile(string configFile)
         {
             configFile = FileSystem.GetFullPath(configFile);

source/Octopus.Cli/Commands/Channel/CreateChannelCommand.cs

@@ -17,12 +17,12 @@ public CreateChannelCommand(IOctopusAsyncRepositoryFactory repositoryFactory, IO
             : base(clientFactory, repositoryFactory, fileSystem, commandOutputProvider)
         {
             var options = Options.For("Create");
-            options.Add("project=", "The name of the project in which to create the channel", p => projectName = p);
-            options.Add("channel=", "The name of the channel to create", s => channelName = s);
-            options.Add("description=", "[Optional] A description of the channel", d => channelDescription = d);
-            options.Add("lifecycle=", "[Optional] if specified, the name of the lifecycle to use for promoting releases through this channel, otherwise this channel will inherit the project lifecycle", l => lifecycleName = l);
-            options.Add("make-default-channel", "[Optional, Flag] if specified, set the new channel to be the default channel replacing any existing default channel", _ => makeDefaultChannel = true);
-            options.Add("update-existing", "[Optional, Flag] if specified, updates the matching channel if it already exists, otherwise this command will fail if a matching channel already exists", _ => updateExisting = true);
+            options.Add<string>("project=", "The name of the project in which to create the channel", p => projectName = p);
+            options.Add<string>("channel=", "The name of the channel to create", s => channelName = s);
+            options.Add<string>("description=", "[Optional] A description of the channel", d => channelDescription = d);
+            options.Add<string>("lifecycle=", "[Optional] if specified, the name of the lifecycle to use for promoting releases through this channel, otherwise this channel will inherit the project lifecycle", l => lifecycleName = l);
+            options.Add<bool>("make-default-channel", "[Optional, Flag] if specified, set the new channel to be the default channel replacing any existing default channel", _ => makeDefaultChannel = true);
+            options.Add<bool>("update-existing", "[Optional, Flag] if specified, updates the matching channel if it already exists, otherwise this command will fail if a matching channel already exists", _ => updateExisting = true);
         }
 
         string channelName;

source/Octopus.Cli/Commands/CommandBase.cs

@@ -20,13 +20,12 @@ protected CommandBase(ICommandOutputProvider commandOutputProvider)
             this.commandOutputProvider = commandOutputProvider;
 
             var options = Options.For("Common options");
-            options.Add("help", "[Optional] Print help for a command", x => printHelp = true);
-            options.Add("helpOutputFormat=", "[Optional] Output format for help, only valid option is json", SetHelpOutputFormat);
+            options.Add<bool>("help", "[Optional] Print help for a command", x => printHelp = true);
+            options.Add<OutputFormat>("helpOutputFormat=", $"[Optional] Output format for help, valid options are {Enum.GetNames(typeof(OutputFormat)).ReadableJoin("or")}", s => HelpOutputFormat = s);
             formattedOutputInstance = this as ISupportFormattedOutput;
             if (formattedOutputInstance != null)
             {
-                options.Add("outputFormat=", "[Optional] Output format, only valid option is json",
-                    SetOutputFormat);
+                options.Add<OutputFormat>("outputFormat=", $"[Optional] Output format, valid options are {Enum.GetNames(typeof(OutputFormat)).ReadableJoin("or")}", s => OutputFormat = s);
             }
             else
             {
@@ -40,6 +39,8 @@ protected CommandBase(ICommandOutputProvider commandOutputProvider)
 
         public OutputFormat HelpOutputFormat { get; set; }
 
+        public abstract Task Execute(string[] commandLineArguments);
+
         public virtual void GetHelp(TextWriter writer, string[] args)
         {
             var typeInfo = this.GetType().GetTypeInfo();
@@ -69,24 +70,6 @@ public virtual void GetHelp(TextWriter writer, string[] args)
             }
         }
 
-        public abstract Task Execute(string[] commandLineArguments);
-
-        private void SetOutputFormat(string s)
-        {
-            OutputFormat = ParseOutputFormat(s);
-        }
-
-        private void SetHelpOutputFormat(string s)
-        {
-            HelpOutputFormat = ParseOutputFormat(s);
-        }
-
-        private OutputFormat ParseOutputFormat(string s)
-        {
-            OutputFormat outputFormat;
-            return Enum.TryParse(s, true, out outputFormat) ? outputFormat : OutputFormat.Default;
-        }
-
         protected virtual void PrintDefaultHelpOutput(TextWriter writer, string executable, string commandName, string description)
         {
             commandOutputProvider.PrintCommandHelpHeader(executable, commandName, description, writer);
@@ -107,8 +90,10 @@ private void PrintJsonHelpOutput(TextWriter writer, string commandName, string d
                         Name = p.Names.First(),
                         Usage = string.Format("{0}{1}{2}", p.Prototype.Length == 1 ? "-" : "--", p.Prototype,
                             p.Prototype.EndsWith("=") ? "VALUE" : string.Empty),
-                        Value = p.OptionValueType.ToString(),
-                        p.Description
+                        p.Description,
+                        Type = p.Type.Name,
+                        Sensitive = p.Sensitive ? (bool?)true : null,
+                        Values = (p.Type.IsEnum) ? Enum.GetNames(p.Type) : null 
                     })
                 })
             }, writer);

source/Octopus.Cli/Commands/Deployment/CreateAutoDeployOverrideCommand.cs

@@ -26,16 +26,16 @@ public CreateAutoDeployOverrideCommand(IOctopusAsyncRepositoryFactory repository
             base(clientFactory, repositoryFactory, fileSystem, commandOutputProvider)
         {
             var options = Options.For("Auto deploy release override");
-            options.Add("project=", "Name of the project", v => ProjectName = v);
-            options.Add("environment=",
+            options.Add<string>("project=", "Name of the project", v => ProjectName = v);
+            options.Add<string>("environment=",
                 "Name of an environment the override will apply to. Specify this argument multiple times to add multiple environments.",
                 v => EnvironmentNames.Add(v));
-            options.Add("version=|releaseNumber=", "Release number to use for auto deployments.",
+            options.Add<string>("version=|releaseNumber=", "Release number to use for auto deployments.",
                 v => ReleaseVersionNumber = v);
-            options.Add("tenant=",
+            options.Add<string>("tenant=",
                 "[Optional] Name of a tenant the override will apply to. Specify this argument multiple times to add multiple tenants or use `*` wildcard for all tenants.",
                 t => TenantNames.Add(t));
-            options.Add("tenantTag=",
+            options.Add<string>("tenantTag=",
                 "[Optional] A tenant tag used to match tenants that the override will apply to. Specify this argument multiple times to add multiple tenant tags",
                 tt => TenantTags.Add(tt));