Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spike showing new CLI usage - got one command working #2263

Closed
wants to merge 2 commits into from

Conversation

dazinator
Copy link
Member

@dazinator dazinator commented May 8, 2020

As part of the refactoring discussed in #2262

Adopting a new CLI parser library could really help simplify that stuff and give us a nice framework build / add commands around for V6.

To this end, I've got a quick spike up and running where I have got one command running: gitversion calculate using @arturcic 's fave CLI parser library: https://github.com/dotnet/command-line-api

Description

  • I unloaded the GitVersion.MsBuild task project. from the solution. I am not concerned with this for this spike.
  • I removed logic to do with remote repo's - I figured users could clone down their own repo's and then run gitversion calculate --normalize on it. Or in a pinch we can add that command later.
  • The only command implemented is gitversion calculate
  • You also get a free help screen and perhaps other features provided by the parsing library.

Help screen (auto created)

image

And here is the command working:

image

I've done it in a way that commands can benefit from DI:

Outline of pattern

Each command is registered for DI in Program.cs

                    services.AddSingleton<GitVersionRootCommand>();
                    services.AddSingleton<CalculateCommand>();

A command looks like this:

 public class CalculateCommand : Command
    {
        private readonly IGitVersionTool gitversionTool;
        private readonly Logging.IConsole console;

        public CalculateCommand(IGitVersionTool gitversionTool, Logging.IConsole console) : base("calculate", "Calculates version information from your git repository")
        {
            this.gitversionTool = gitversionTool;
            this.console = console;
            this.AddOption(new Option<bool>(
            "--normalize",
            "Attempt to mutate your git repository so gitversion has enough information (local branches, commit history etc) to calculate."));
            this.Handler = CommandHandler.Create<bool?>(ExecuteAsync);            
        }

        private async Task ExecuteAsync(bool? normalize)
        {
            if (normalize ?? false)
            {
                await Normalize();
            }

            var variables = this.gitversionTool.CalculateVersionVariables();
            console.WriteLine(variables.ToString());
        }

        private Task Normalize()
        {
            throw new NotImplementedException();
        }
    }

I tried to keep as much of everything else the same, but had to make some quick adjustments, to have the command actually executed.

Motivation and Context

Gives us a way to structure commands, and a modular way to add new commands, backed by a parsing library that we don't have to maintain ourselves and offers more powerful features beyond the primary concern of gitversion. Establishing something like this will let us build out the command structure we need for V6 more easily.

How Has This Been Tested?

I did enough to get the solution to compile and run one command with the new parser, just as a proof of concept. The changes I made were not thought about very much. I tried to do as little as possible, and made some on the spot decisions. I unloaded projects just to get it to run. The important thing is to try and show some pattern or example and discuss from there before we have to refactor the code base.

@@ -418,7 +418,7 @@ public void GetProjectRootDirectoryWorkingDirectoryWithWorktree()

sp = GetServiceProvider(gitVersionOptions);

gitVersionOptions.ProjectRootDirectory.TrimEnd('/', '\\').ShouldBe(worktreePath);
Copy link
Member Author

@dazinator dazinator May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: ProjectRootDirectory was actually being set to the "working directory" of the git repository. This is different from the command line's working directory, or the directory of the .git folder. I renamed it to GitRepositoryWorkingDirectory to aid understanding whilst refactoring. It's not a critical change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been a constant source of friction, because I think it's used to support dynamic repositories in a weird way I don't quite understand. I think it's been changed back and forth several times in different PR's, each breaking one other use case that isn't well enough tested. 🤷‍♂️

@@ -35,11 +35,9 @@ public void Prepare()
var currentBranch = ResolveCurrentBranch();

var dotGitDirectory = gitVersionOptions.DotGitDirectory;
var projectRoot = gitVersionOptions.ProjectRootDirectory;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This argument wasn't used, and I removed it whilst refactoring because it helped me.

@@ -50,22 +48,17 @@ public void Prepare()
private void PrepareInternal(bool normalizeGitDirectory, string currentBranch, bool shouldCleanUpRemotes = false)
{
var gitVersionOptions = options.Value;
if (!string.IsNullOrWhiteSpace(gitVersionOptions.RepositoryInfo.TargetUrl))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the dynamic repository stuff as per the comments on this PR. User can clone down their own repo and run gitversion normalise against it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good riddance! 😅

@@ -93,13 +93,14 @@ public void UpdateAssemblyInfo(VersionVariables variables)
{
var gitVersionOptions = options.Value;

if (gitVersionOptions.AssemblyInfo.ShouldUpdate)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can eventually become a seperate tool as discussed.


using var repository = new Repository(dotGitDirectory);
return repository.Info.WorkingDirectory;
//return gitVersionOptions.WorkingDirectory;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought it made more sense to always ask git what the working directory is, rather than assume its the cli 's working directory (current directory) - given we always resolve the dotGitDirectory.

public string DotGitDirectory => dotGitDirectory.Value;
public string ProjectRootDirectory => projectRootDirectory.Value;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ProjectRootDirectory --> renamed to GitRepositoryWorkingDirectory for selfish reasons.

@dazinator
Copy link
Member Author

I just noticed I got half way through trying to show an example of how a global cli option works and didn't finish. I'll push this change up tomorrow. It was basically to set up '--logging-method Console | File' (see the GitVersionRootCommand) as a "global option" that will then have its value accessible to all sub commands so they can log accordingly (to console or file etc). Thought we might need that in a few places.

@arturcic
Copy link
Member

arturcic commented May 9, 2020

Wow @dazinator it's awesome. I did not go through all the changes, but from it seems to be great. We need to fix the bugs planned for 5.3.x and we can focus on release 6.0

//this.AddGlobalOption(new Option("--target-path") { Argument = new Argument<LoggingMethod>() });
//this.AddGlobalOption(new Option("--logging-method") { Argument = new Argument<LoggingMethod>() });
GlobalOptions = globalOptions.Value;
this.AddCommand(calculateCommand);
Copy link
Member

@arturcic arturcic May 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So rignt now we construct the command tree by adding command like this.

Now wonder if we can add these commands dynamically, I mean we can register in the DI several commands and then we can resolve them in constructor as IEnumerable that will get added to the rootCommand. This will allow us to inject commands from external dll loaded as plugin/addin.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the idea is you have a assembly info update tool as plugin for gitversion. You define in the tool that you have a command assemblyInfo with it's options/arguments. Then in the bootstrapping phase of gitversion you register that command in DI. The Root command gets the list of available commands from DI and this way you have assembly info command support. Can we add this to this PR? just as a sample?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we ship something "in the box", i.e. the ability to update an assemblyinfo file, but written in such a way that it can be replaced by a 3rd party plugin? Or should we refactor it out completely, to then have the end-user configure what they want to use?

Personally, I prefer the latter, but I do worry that people will then be confused about how to go from 5.x to 6.x.

I like things being separate, which helps with the updating of those compontents. But how do we make it so that users moving from 5.x to 6.x, have to do limited setup, configuration, to get things working how they were before?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well @gep13 you are right, we need to think about the migration path from 5.x to 6.0. We might want to have something in the box. Or we can have them out of the box, and somehow enable the previous behavior

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But for now we can play with the idea

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the latter, being able to enable the previous behaviour, just not sure what the best way to do that is ☹️

Copy link
Member Author

@dazinator dazinator May 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So rignt now we construct the command tree by adding command like this.

Now wonder if we can add these commands dynamically, I mean we can register in the DI several commands and then we can resolve them in constructor as IEnumerable that will get added to the rootCommand. This will allow us to inject commands from external dll loaded as plugin/addin.

Yep, or we can also resolve addin commands separately and add them to the root command after its constructed. The only reason for me adding the explicit command types as constructor arguments is I thought it made it a bit clearer to see how the command structure looked and would error if you forgot to register a command that we saw as a necessary / non optional dependency, with optional or add in commands being added after construction.

@dazinator
Copy link
Member Author

dazinator commented May 9, 2020

One issue I am working through is how to use global options.
Some options will be global to all subcommands, like whether to log to console or a file for example. At the moment, in GitVersionExecutor we rely on already parsing the arguments, so we can set up logging, before actually executing the command.

            if (gitVersionOptions.LogToConsole)
            {
                log.AddLogAppender(new ConsoleAppender());
            }
            else if (gitVersionOptions.LogFilePath != null && gitVersionOptions.LogFilePath != "console")
            {
                log.AddLogAppender(new FileAppender(gitVersionOptions.LogFilePath));
            }

However using the new library, there isn't an intial "parse step" that I can see, - you just execute a command, providing the args, and it will then parse the args, then call you back by actually executing your command handler and passing in the option values you have configured.

The new pattern then (I could me missing something at this library we are using is still pretty new and not everything seems to have been documented yet), looks to be a case of defining these global options on the root command like so - here I have set up a global option called "--logging-method" which can take an Enum value LoggingMethod which currently has Console or File.

 public GitVersionRootCommand(CalculateCommand calculateCommand) : base("Versioning for your git repository, solved!")
        {             
            this.AddGlobalOption(new Option("--logging-method") { Argument = new Argument<LoggingMethod>() });         
            this.AddCommand(calculateCommand);
        
        }

And then, the first location that can take advantage of this is the handler for each subcommand - for example here is the Calculate command and you can see we can now inject LoggingMethod into the ExecuteAsync as it will be bound to the global option:

 public class CalculateCommand : Command
    {
        private readonly IGitVersionTool gitversionTool;
        private readonly Logging.IConsole console;

        public CalculateCommand(IGitVersionTool gitversionTool, Logging.IConsole console) : base("calculate", "Calculates version information from your git repository")
        {
            this.gitversionTool = gitversionTool;
            this.console = console;
            this.AddOption(new Option<bool>(
            "--normalize",
            "Attempt to mutate your git repository so gitversion has enough information (local branches, commit history etc) to calculate."));
            this.Handler = CommandHandler.Create<bool?, LoggingMethod>(ExecuteAsync);            
        }

        private async Task ExecuteAsync(bool? normalize, LoggingMethod loggingMethod)
        {
            if (normalize ?? false)
            {
                await Normalize();
            }

            if(loggingMethod == LoggingMethod.Console)
            {
                // etc
            }

            var variables = this.gitversionTool.CalculateVersionVariables();
            console.WriteLine(variables.ToString());
        }

So it's a case of working out a new place to deal with the global options in a centralised mannor. Currently it looks like that would mean we'd potentially derive all of our sub commands from a common base that can be called to process the global options:

I'll push up the example of that, but thought it worth raising in case there is meant to be a different way to handle this that someone can see.

@arturcic
Copy link
Member

arturcic commented May 9, 2020

@dazinator do you mind if I convert the PR to draft?

@dazinator
Copy link
Member Author

@arturcic yep thats fine, this is dead now, I'll leave this just for reference although most of this was moved to the new branch

@dazinator dazinator closed this May 10, 2020
@arturcic
Copy link
Member

arturcic commented May 10, 2020

Thanks, makes sense, let's continue on the branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants