Conversation
As of this commit: These still need a larger scope refactor due to a non standard options or calling pattern: Defog Detect Backdoor Detect Crypto Find Squats Metadata Also, even those implemented mostly need to return a proper error code isntead of void Task.
They now are part of the CLI lib
OctoKit dependency changed this field from int to long so updating model to match.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
danfiedler-msft
left a comment
danfiedler-msft
left a comment
There was a problem hiding this comment.
Overall, these changes look good to me.
My personal preference would be to rename the LegacyRunAsync methods to reflect what the method does. The RunAsync method makes sense because it comes from the base class and must apply to all commands but there's no reason the LegacyRunAsync methods can't be renamed to be more specific. It isn't worth holding the PR up over.
There was a problem hiding this comment.
nit: It appears that git believes this is a new file rather than rename of oss-download/DownloadTool.cs so we'll lose the history on it. (It is probably not worth going back through commit history to fix this.)
| using Microsoft.CST.OpenSource.OssGadget.Tools.HealthTool; | ||
| using System.Reflection; | ||
|
|
||
| class OssGadgetCli : OSSGadget |
There was a problem hiding this comment.
nit: it is more .NET idiomatic to have the class and filename match (i.e., class Program in Program.cs) which would move OssGadgetCli to a separate file.
There was a problem hiding this comment.
Renamed program.cs to ossgadgetcli.cs.
| { | ||
| var cli = new OssGadgetCli(); | ||
| await cli.ExecuteAsync(args); | ||
| return (int)cli._returnCode; |
There was a problem hiding this comment.
nit: it is odd to use a field for a return value. We could refactor ExecuteAsync (and the methods it calls) to return this value instead.
There was a problem hiding this comment.
Good point. I've simplified this.
|
I think the different verbs/options For example, "Calculate a risk metric for the given package" provides more information to the user than "Run risk calculator tool" Any help text that says, "runs the Foo tool" makes me ask "...but what does Foo tool do?" |
3 participants
This PR mainly refactors the many CLI projects to a single entrypoint. From a CLI perspective they now can be accessed with verbs against a the single
oss-gadget-clibinary. This also standardizes calling patterns for each tool using theBaseToolclass to aRunAsyncmethod which takes the appropriate type of options. If the project previously had aRunAsyncentrypoint with a different return type the old entrypoint was renamedLegacyRunAsync. This refactor does not attempt to change execution behavior, merely to simplify project structure and publishing, specifically to enable publishing the single CLI binary to Nuget withotu having to manage many project publications.Also updates all dependencies.
Changes Model.User.Id field to long (from int) to match changes in OctoKit library which populates that field.
Marked as draft for feedback and refactor of build pipeline.