Uh oh!
There was an error while loading. Please reload this page.
Add support for Listing and Removing API Keys - #27
Conversation
nblumhardt
left a comment
There was a problem hiding this comment.
Looking good! Added a bunch of thoughts since it's the first feature like this and there's not much existing code to compare with, keen to know your thoughts. Cheers!
| namespace SeqCli.Cli.Commands.ApiKey | ||
| { | ||
| [Command("apikey", "list", "Send a structured log event to the server", Example = | ||
| "seqcli log -m 'Hello, {Name}!' -p Name=World -p App=Test")] |
There was a problem hiding this comment.
Example/description is out of sync here
| } | ||
| await connection.ApiKeys.RemoveAsync(apiKeyToRemove); | ||
| Console.WriteLine($"\"{_title}\" API Key removed"); |
There was a problem hiding this comment.
So far in this project we've taken the "unix" approach of not writing any output on success; keen to see how that goes, would be good to line this up.
There was a problem hiding this comment.
Sounds good. I will remove the line.
| _connection = Enable<ConnectionFeature>(); | ||
| Options.Add( | ||
| "t=|title=", | ||
| "", |
| { | ||
| _connectionFactory = connectionFactory; | ||
| _connection = Enable<ConnectionFeature>(); | ||
| Options.Add( |
There was a problem hiding this comment.
Is id a worthwhile option/alternative to title? Would help with the ambiguous ones where names overlap.
There was a problem hiding this comment.
I have added Id and put a check in to ensure that they only specify a single value since both could be confusing as to if its removing only when both match or just one.
| var connection = _connectionFactory.Connect(_connection); | ||
| var apiKeys = await connection.ApiKeys.ListAsync(); | ||
| var apiKeyToRemove = apiKeys.FirstOrDefault(ak => ak.Title == _title); |
There was a problem hiding this comment.
Would it make as much sense to remove all matches, instead of just the first, here?
There was a problem hiding this comment.
Ahh. I completely forgot you can have multiple with the same Title. I will fix that up.
| }); | ||
| foreach (var apiKey in data) | ||
| { | ||
| var apiKeyString = JsonConvert.SerializeObject(apiKey); |
| Log.Debug("Retrieved ApiKeys {@ApiKeys}", apiKeys); | ||
| var data = apiKeys.Select(a => new | ||
| { | ||
| a.AppliedProperties, |
There was a problem hiding this comment.
If we listed Id and Title first here, it might be a bit quicker to visually parse the output.
andymac4182
commented
Feb 26, 2018
Thanks for the feedback @nblumhardt I wasn't sure exactly the best way to approach a few of these but this has given me good direction. I will try get this updated shortly. |
| namespace SeqCli.Cli.Commands.ApiKey | ||
| { | ||
| [Command("apikey", "list", "Send a structured log event to the server", Example = |
There was a problem hiding this comment.
The description of the command should be updated here :)
There was a problem hiding this comment.
Thanks @tsimbalar I have fixed that up now.
nblumhardt
commented
Feb 27, 2018
👍 We might want to think a bit more about how output/formatting could work consistently for multiple entity types, e.g. maybe we output plain text (or colorized CSV?) by default, and use The output could also be made Seq-version-agnostic by side-stepping the strongly-typed Tough balance to strike between ergonomics and automation-friendliness, should be fun :-) |
This is an initial idea for listing and removing of API Keys it is based on my work in #26. I plan on adding support for adding api keys.