Added support for per target CLI flags/params

[mojzesh]

Hello Guys,

I’ve been using Mage for more than a month now, for some serious work and I can say it works brilliantly.

In the past I was using a lot of Make and the only thing I'm missing from Make in Mage is the ability to pass arguments to targets.

For example, in Makefile I can do something like this:

ARG=
target:
     action $(ARG)

and then call it from CLI like:

make target ARG=foo

I know I can use environmental variables for it and it works, but in my opinion it's a bit counter-intuitive when you want to build a nice tool using Mage which will allow user to work in natural fashion by first typing a command name and then feeding it with some arguments/params.

Clearly I'm not the only one who is missing this feature (according to these posts):

#24
#24 (comment)

Over the weekend I came up with my own solution to this problem. At first I was trying simply to obtain args using os.Args at target "scope" but it didn't work, as all of passed args were removed, and also immediately I was given errors about mismatched target names (because args passed to targer were interpreted as targets names obviously).

After some reverse engineering I found out that at Mage code-level all args are delivered by the os nicely, but they get removed during Mage initial processing.

So in my solution I modified the template.go file in order to prevent arguments removal and also to prevent error about "unknown targets" so that passed args are no longer interpreted as targets names.

Each processed argument gets marked with prefixes based on the name of target which was preceding it. Because targets args are coming after targets names, we can know which args belong to which target.
Also, my code is dealing correctly with target aliases; aliases are resolved to full original target name and stored as such in a prefix instead. Those names have to later match with caller function detection (more about this later).

The following flags format are supported:

// --                                          : stop processing any further arguments
// -f --f -flag --flag                         : boolean flags only
// -f=value --f=value -flag=value --flag=value : boolean, integer, string flags
// f=value flag=value                          : boolean, integer, string flags (read above below note)

Those formats are matched using below RegEx:

(^\w+\=\S*)|(^-{1,2}\w+\=\S*)|(^-{1,2}\w+$)

(it can be validated here: https://regex101.com/)

Given that we have three targets:

new
build
run

General format:

mage target1 [FLAGS] target2 [FLAGS] ...

examples:

mage target1 --boolean-flag --long-flag=abc -r=30 -v target2 --some-other-value="5a23d6eb3bdbcd6" -vv var=123

mage new
mage new --name=banana --input='/some/path/to/file.ext' --enabled -r
mage new --name=banana --input='/some/path/to/file.ext' --enabled -r build stage=dev -n=3 run

There is also support provided for -- which is causing Mage to stop processing any further CLI arguments or running any further targets:
in the following example, only new and build target will be called, and build will only get stage argument, but not n argument for example.

(Note: please notice -- after stage=dev)

mage new --name=banana --input='/some/path/to/file.ext' --enabled -r build stage=dev -- -n=3 run

Some examples:

-f
-1
-flag
--f
--1
--flag
-a="/path/to/file" --nextFlag
-1="/path/to/file"
-flag="/path/to/file"
--a="/path/to/file"
--1="/path/to/file"
--flag="/path/to/file"
-f=
-1=
-f=3 --nextFlag
-1=abc
-flag=cheese
--f=
--f=300millions
--1=awesomeness
--flag="/path/to/file"
var=
var=some
other_var=something

These cases are ignored by this RegEx:

-
--
target
another_target
---b
---banana

Processing args at example "New" target

In the below snippet the crucial part of the way it works
is this function call:

mg.GetArgsForTarget()

This function is checking callstack and finds out the name of the caller function,
then it's checking all CLI args available at os.Args and finds those which contains caller function
name in it (e.g.: main.new: ). Then it returns the list of all those found args as strings array
which can then be processed by flags package as follows:

mage new --name=banana --input='/some/path/to/banana.jpg' --enabled -r=30

type NewOptions struct {
    Name    string
    Input   string
    Enabled bool
    R       int
}

func New() {
    var defaultOptions NewOptions
    options := defaultOptions
    //----------------------------
    // Here we call our args filtering
    // function, which will give us
    // only arguments applying to "New" target
    //----------------------------
    args := mg.GetArgsForTarget()
    //----------------------------
    // process args using flags package, and do whatever you want
    fs := flag.NewFlagSet("mytool new", flag.ExitOnError)
    fs.StringVar(&options.Name, "name", "models", "resource name")
    fs.StringVar(&options.Name, "n", "models", "resource name")
    fs.StringVar(&options.Input, "input", "", "resource path")
    fs.StringVar(&options.Input, "i", "", "resource path")
    fs.BoolVar(&options.Enabled, "enabled", false, "something is enabled")
    fs.BoolVar(&options.Enabled, "e", false, "something is enabled")
    fs.IntVar(&options.R, "r", 0, "radius")
    fs.Parse(args)
    //----------------------------
    // Deal with any parsed args
    // as object nicely here
    //----------------------------
    fmt.Println("Created New Project!")
    fmt.Println("Name: ", options.Name)
    fmt.Println("Input: ", options.Input)

ps.
I was testing this code a lot, and it's working for me.
Let me know what you think about this change and I'll write tests for it :)

[mojzesh]

Hello again guys!
Can someone review this PR please ?
It's been 8 days since I made it.
Thanks in advance...

[natefinch]

Hi, sorry for the delay, and thank you very much for the PR.

This looks like a really good first step - I had thought to generate the flag parsing code according to the function args, but that's a much bigger change. I like the "magic" helper to get the appropriate arg list. I'll take a look at the tests probably tomorrow... I think there are edge cases that could be problematic (which is one of the reasons I haven't tackled this code yet), but maybe it's not actually a problem with this implementation.

[natefinch]

So, I think there has to be a better way to do this rather than modifying os.Args. I think we can just build up a slice of the args for every target called, and GetArgsForTarget could return that. I don't like writing custom flag parsing code either. This seems like it's a lot of work just to support foo=bar, when the default flags package supports -foo=bar, which seems good enough for me.

[mojzesh]

Hi Nate.
Ok, thanks for this, I'm fine with your suggestions.
I'll try to implement it today evening.

[arturtorun]

Hi again Nate.

Sorry for the delay, had a busy week at work. I pushed another commit with simplified logic and new approach.

I've added some more information to mage CLI Usage about how to use flags (maybe it's worth adding some information there about how to pass Namespaces and Aliases also ?). Also as you suggested I removed support for foo=bar kind of flags.

In current approach I decided to pass flags through the context, so instead of using helper function:

flags := mg.GetArgsForTarget()

We can now use this one (which require context as its input):

flags := mg.GetFlagsFromContext(ctx)

Basically, I'm collecting all targets flags in map[string][]string, so for each target name I have slice of it's arguments. Then based on target name in the target execution loop I'm injecting those arrays into each corresponding target's context.

In parse.go I've added one line to two of the snippets which carry ctx variable in order to inject flags slice into context:

wrapFn := func(ctx context.Context) error {
    ctx = context.WithValue(ctx, "flags", perTargetFlagsMap["%s"])
    %s(ctx)
    return nil
}

I've tested this with plain functions targets, with Aliases, and Namespaces ( defined in same go package and imported from another package using mage import decorator //mage:import) and all works smoothly :)

The only corner case problem I've found so far is that if you set a function to be default (say version target in below example, then you obviously can call it like:

mage 
>$ version: 1.0.0

or

mytool
>$ version: 1.0.0

but if that default function require some arguments then if you try to do something like this:

mage --myflag

then Flags package is displaying mage/command Usage, so it will never reach the loop of target processing.
But obviously you can still do something like this and version flag will be able to get --myflag flag:

mage version --myflag

I believe it's not a big problem. It's fixable but In order to fix it we could either do custom parsing of mage flags and once we have information that there is a default target set by user instead of displaying usage/error we can proceed with execution of that one default target with given flags, so that it will have actual opportunity to use that flag/s.

Anyway, let me know what do you think about this approach, and as I said I'll write the tests for it... (although I tested this extensively in various combinations)

[mojzesh]

Hi Nate!

Any timescale on reviewing my last PR ?
It's blocking my other projects a bit.
Thanks.

[natefinch]

Yeah, let me look at this, I may have time tonight. Sorry for the delay, it's crunch mode at work and free time has been limited.

[mojzesh]

Hi again Nate!

Did you (by any chance) had some time to look at my PR ?
Thanks.

[natefinch]

looking now. That regex scares me, btw.

[natefinch]

so... I see the innovation here. The innovation being that you don't allow --foo bar to mean "foo has a value of bar". And thus you don't have to wonder if foo is a boolean variable, with bar being a target, or if foo is a string variable with the value of bar.

This has a lot of potential.

[mojzesh]

Oh yes! I love RegEx'es :) and I'm using
https://regex101.com a lot while working on them...

I'm glad you like my solution.
I'll write some tests for this PR over the weekend.
Thanks!

[natefinch]

So, this feels like a half-step for me. Manually having to extract the list of arguments is not really that much better than setting environment variables like foo=bar mage doit and then using os.Getenv to get those values.

What I would really like is to allow people to specify normal arguments to mage targets, and have them settable with flags.

Like

func Build(os string) error {

And then you'd call mage build --os=windows and it would Just Work.

So I think this is a good start, but it needs some more tweaking... and I think I want to get others' input on such a proposal first.

[taliesins]

@natefinch Could we not get this merged, and the next stage in the evolution is to get it to func Build(os string) error {

Its a real pain on windows cmd to work with passing arguments as environment variables

[natefinch]

I plan to start working on argument support now. I think I'd prefer to do the full feature rather than have to support half the feature for the forseeable future.