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

auto-args for targets? #24

Closed
natefinch opened this issue Oct 1, 2017 · 28 comments
Closed

auto-args for targets? #24

natefinch opened this issue Oct 1, 2017 · 28 comments

Comments

@natefinch
Copy link
Member

It wouldn't be too hard to auto-generate a CLI for each target so the user could pass in positional args or flags per arg.

e.g.

func Build(OS string, ARCH string) error {
     // build with GOOS and GOARCH
}

$ mage build -os darwin -arch amd64
building with GOOS=darwin GOARCH=amd64
@natefinch
Copy link
Member Author

May conflict with #34

@jrnt30
Copy link

jrnt30 commented Oct 6, 2017

While it's a little ugly, I think you could support both features if you kept the multi-target support to be explicitly delimited. The available bind targets then for the method arguments would be the union of the two. If both targets had an OS string in them, they would both be passed the same value.

@natefinch
Copy link
Member Author

I see . what you mean, like if build had an OS string argument, and compile did too, but compile also had a output string then you'd call it like

mage build compile -OS=windows -output=foo.exe

That's not terrible :)

@vassudanagunta
Copy link

mage build compile -OS=windows -output=foo.exe

While it's a little ugly

That's not ugly, it's beautiful! And elegant if targets declare a same named arg and you only have to specify it once on the command line. Or where you thinking of some other approach, @jrnt30?

@jrnt30
Copy link

jrnt30 commented Dec 31, 2017

Now that was the general thought I had a well

@natefinch natefinch added this to the Grem123 milestone Jun 22, 2018
@sean-
Copy link

sean- commented Aug 22, 2018

I don't know about coupling the variables to the positional args for a given target. Wouldn't most use cases be satisfied with something closer to:

type Args map[string][]string

func Build(args Args) error {
  switch {
  case args.Get("GOOS") && args.Get("GOARCH"):
    // build with GOOS and GOARCH
  default:
    // other
  }
}

// Get returns the first argument
func (a Args) Get(key string) string { ... }

The interface is basically ripped off from https://golang.org/pkg/net/url/#Values.Get ? This solves a huge list of problems on our end with mage and would be a very, very welcome feature. Maybe return the args as part of a context.Context so we don't need to change the function signature for anything. ?

@natefinch
Copy link
Member Author

I think the question is mainly how they're specified on the command line and how that interacts / interferes with calling multiple targets.

Getting args into the target itself is easy enough. Right now, you can do

GOOS=windows mage build
and then use os.Getenv("GOOS") to retrieve the value.

But defining the args in the target function's arguments is more elegant and lets us validate them automatically, produce docs for them, etc.

The only problem is how they look when you specify them on the command line, how or if we disambiguate between two targets' args, etc

@sean-
Copy link

sean- commented Aug 22, 2018

That's true, too. I think if you have multiple targets, you have to scan from left to right and parse the args between the targets and use that as the rough function signature. Does order matter? That's problematic UX. How do you pre-register an argument so it's available as help? What if a target has 10 flags available and half of them are optional, then what? Pass them all in as arguments to the target func? Use a pointer to a string?

func Foo(barRequired string, bazOptional *string) error {

? but times however many args? The problem I have with using a container like a map[string][]string is how do you pre-register the args for a given flag so a --help dialogue can be populated (and provide the necessary description and/or code reuse across different targets)?

@davidmz
Copy link

davidmz commented Sep 11, 2018

Could you, for a start, just allow additional arguments after the targets? For now, mage test -xxx returns error Unknown target specified: -xxx. But obviously, the target name can not start with dash. So you can stop search targets after the first dash-started argument and treat it and the following arguments as target-related. User then can parse these arguments himself in magefile code.

@davidmz
Copy link

davidmz commented Sep 11, 2018

As for the multiple targets, I would suggest to use prefixes: mage build compile -OS=windows -compile:output=foo.exe. In this example -OS is the argument for both 'build' and 'compile', and -output is for 'compile' only. But it can be realized via the separate helper library.

@lucsky
Copy link

lucsky commented Sep 26, 2018

Disclaimer: I absolutely did not think this through, this is basically a quick brain dump to maybe fuel the conversation.

How about using structs to define global and target specific arguments ?

Something like this:

// +build mage

package main

import "fmt"

type globalParams struct {
	Verbose bool `default:false short:v`
}

type uploadParams struct {
	Source string
	Target string `default:"someURL"`
	Retry  *int
}

// Default is the default target when invoking mage without an explicit target
var Default = Upload

// GlobalParams maps the global options that can be passed from command line
var GlobalParams = globalParams{}

// Hello says hello
func Hello() {
	if GlobalParams.Verbose {
		fmt.Println("Hello World")
	}
}

// Upload sends stuff to some place
func Upload(params uploadParams) {
	if GlobalParams.Verbose {
		fmt.Printf("Uploading %s to %s...\n", params.Source, params.Target)
	}
}

Using a struct would allow to:

  • map arguments type
  • document arguments using either comments or struct tags
  • use struct tags for even more options
  • not require fixed arguments position

In the snippet above for example, GlobalParams.Verbose is required (because it cannot be nil) but can be omitted because it has a default value defined as a tag, as well as a short form.

So that would allow:

  • mage
  • mage --verbose
  • mage -v

The uploadParams struct on the other hands defines target specific arguments where:

  • Source is required
  • Target is required but can be omitted because a default value is defined
  • Retry is an optional int argument

So these combinations would be valid:

  • mage upload --source=foo --target=bar
  • mage upload --source=foo --retry --verbose
  • mage -v upload --source=foo --retry=3

I kinda like the fact that this is more explicit than magic 😊

@cking
Copy link

cking commented Mar 26, 2019

Sorry for necromancy

Many Unix tool stop parsing command line args after encountering --.
How about doing that as well, and just providing the stuff after it as a function or var of the mg module.

That way, you don't have to bother about deciding on a format to choose, it's the task of the Magefile writer.

Just my 2c (I just stumbled upon a situation where I could use something like this)

@natefinch
Copy link
Member Author

hmmm... that's a really interesting idea. I don't think it's a full solution, but it may be good enough for a start, and it gives windows users a better way to get data into a script, without having to do wacky things (since they can't do FOO=bar mage build AFAIK).

@natefinch
Copy link
Member Author

Also - there's no such thing as necromancy for open issues. Open is open :)

@faxal
Copy link

faxal commented Dec 10, 2019

I keep coming back to this issue whenever I start writing new tasks. This would greatly simplify a lot of things.

I wanted to propose a pattern that is used in the wild in Devops world:

This is how ansible does it:

ansible-playbook release.yml --extra-vars "version=1.23.45 other_variable=foo"

And this is how terraform does it:

terraform apply -var="image_id=ami-abc123"

I like how terraform does it and I think we could adopt that for mage. I feel it's a good enough solution

mage build -var os=darwin -var arch=amd64

@natefinch
Copy link
Member Author

Hmm I kind of like that. It's nice not to have to generate new flag handling based on what target you're calling. One flag to contain all variables is nice.

@cking
Copy link

cking commented Dec 10, 2019

depending on the format though you might add more suphisticated parsing. not that its very hard to do, but still

@dylanmei
Copy link

I'd like to add that this is how jsonnet does this

Available options for specifying values of 'top-level arguments':
Provide the value as a string:
  -A / --tla-str <var>[=<val>]     If <val> is omitted, get from environment var <var>
       --tla-str-file <var>=<file> Read the string from the file

@cking
Copy link

cking commented Dec 10, 2019

for reading vars from files, i'd prefer one of the dotenv solutions

@dylanmei
Copy link

dylanmei commented Dec 10, 2019

Explicit file in addition to env substitution has its uses, for example when using mage to orchestrate infrastructure workflows or materialize templates with lots of vars from ci/cd.

One flag to contain all variables is nice

Based on that preference, consider a slight variation on Terraform that might look like this

mage build -vars "os=darwin arch=amd64" -vars-file "extra.vars"

@josharian
Copy link

One flag to contain all variables is nice

It is certainly simpler to implement. It's not obvious to me that it is nicer to use.

@natefinch
Copy link
Member Author

Hmm actually it's not as big of a help as I was thinking... because to make it actually easy, it would need to be
mage -vars="os=darwin arch=amd64" build
i.e. -vars has to come before the target name, which is obviously not great.

I think we can do it a nicer way where you can do

mage build -os=darwin -arch=amd64

and it'll do the right thing.

@cking
Copy link

cking commented Dec 10, 2019

well, you could create a second flags instance (beside flags.commandline) that will read flags.args()[1:] to create a second parse using the flags coming after your flags.args()[0] entry.

@kpym
Copy link

kpym commented Sep 5, 2020

Probably a stupid proposition, but why not simply use mage build(darwin,amd64) as interface for the arguments ?
Obviously there is a problem if the argument has comma inside but this can be overcome in several ways.
In this case mage task and mage task() should probably be equivalent.

@wenerme
Copy link

wenerme commented Sep 5, 2020

Prefer mage build -os=darwin -arch=amd64, but pass the rest of flags to context or something, let target parse the flags.

@natefinch
Copy link
Member Author

Done! I went with "just do positional args, ya dummy" and I think it worked out awesomely.

@sheldonhull
Copy link

hmmm... that's a really interesting idea. I don't think it's a full solution, but it may be good enough for a start, and it gives windows users a better way to get data into a script, without having to do wacky things (since they can't do FOO=bar mage build AFAIK).

Slight variation when I use pwsh.
Easy to test with pwsh on Mac or use codepace/pwsh container. Cross platform. Core syntax like that hasn't changed as far as I know from 5.1 (it's on 7+).

$ENV:Foo='bar'; go build

@DeadlySurgeon
Copy link

Probably a stupid proposition, but why not simply use mage build(darwin,amd64) as interface for the arguments ? Obviously there is a problem if the argument has comma inside but this can be overcome in several ways. In this case mage task and mage task() should probably be equivalent.

Super late to the party, but the reason why you might not want to do that is that you might have other parameters too as well as optional parameters. In the case os and arch, what if 99% of the time you build for the host box, but you still want to be able to build for other architectures? Unless you wanted to have to enter them in every single time despite having sane defaults, you'd have to add a new command, BuildArch, but then that leads into more chaos the more parameters you have, as well as more functions to have to remember.

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

No branches or pull requests