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

Create a v1 => v2 migration guide #921

Closed
coilysiren opened this issue Nov 9, 2019 · 22 comments · Fixed by #1098 or #1137
Closed

Create a v1 => v2 migration guide #921

coilysiren opened this issue Nov 9, 2019 · 22 comments · Fixed by #1098 or #1137
Assignees
Labels
kind/documentation describes a documentation update status/confirmed confirmed to be valid, but work has yet to start

Comments

@coilysiren
Copy link
Member

coilysiren commented Nov 9, 2019

View the release notes for guidance on migrating => https://github.com/urfave/cli/releases/tag/v2.0.0

@coilysiren coilysiren added help wanted please help if you can! kind/documentation describes a documentation update status/confirmed confirmed to be valid, but work has yet to start labels Nov 9, 2019
@coilysiren
Copy link
Member Author

Here's a PR where @ffrank and @purpleidea make the v1 => v2 migration purpleidea/mgmt#571

@coilysiren
Copy link
Member Author

coilysiren commented Nov 13, 2019

Here's a quirky example of someone moving from the very very old v2 branch, to the latest v1 release sourcegraph/godockerize#10

☝️ I don't think that's the type of migration where's really looking to write documentation for? but it's interesting to look at

@coilysiren coilysiren changed the title Create a v1 => v2 migration guide Create a v1 => v2 migration guide Nov 14, 2019
@coilysiren
Copy link
Member Author

Here's another example migration PR google/cloud-print-connector#470

@coilysiren coilysiren pinned this issue Nov 27, 2019
@coilysiren coilysiren self-assigned this Nov 29, 2019
@coilysiren coilysiren added status/claimed someone has claimed this issue and removed help wanted please help if you can! status/confirmed confirmed to be valid, but work has yet to start labels Nov 29, 2019
@coilysiren
Copy link
Member Author

I'm going to work on this as a part of #952

@coilysiren
Copy link
Member Author

In addition to working on this as a part of #952, I've been progressively updating the v2 release notes => https://github.com/urfave/cli/releases/tag/v2.0.0 to include fine grain migration instructions. I'll add more as I find them!

@coilysiren coilysiren unpinned this issue Dec 3, 2019
@coilysiren coilysiren reopened this Dec 4, 2019
@coilysiren coilysiren removed their assignment Dec 4, 2019
@coilysiren coilysiren added status/confirmed confirmed to be valid, but work has yet to start and removed status/claimed someone has claimed this issue labels Dec 4, 2019
@coilysiren coilysiren pinned this issue Dec 4, 2019
@coilysiren
Copy link
Member Author

#952 was closed, so I no longer intend on working on this

@coilysiren
Copy link
Member Author

+1 migration PR => technosophos/dashing#52

@bonedaddy
Copy link

command line flag migration steps also need to be updated, at the very least to document the way command-line flag aliases now behave.

Previously you could define aliases liek:

cli.StringFlag{
  Name: "config, cfg"
}

however you now need to do

cli.StringFlag{
    Name: "config",
    Aliases: []string{"cfg"},
}

Here is a minimal repro

package main

import (
	"fmt"
	"os"

	// auto register pprof handlers

	_ "net/http/pprof"

	au "github.com/logrusorgru/aurora"
	"github.com/urfave/cli/v2"
)

func main() {
	// generate the actual cli app
	app := newApp()
	// runthe cli app
	if err := app.Run(os.Args); err != nil {
		fmt.Printf(
			"%s %s\n",
			au.Bold(au.Red("error encountered:")),
			au.Red(err.Error()),
		)
		os.Exit(1)
	}
}

func newApp() *cli.App {
	app := cli.NewApp()
	app.Flags = loadFlags()
	return app
}

func loadFlags() []cli.Flag {
	return []cli.Flag{
		&cli.BoolFlag{
			Name:  "bootstrap, bp",
			Usage: "bootstrap against public ipfs",
		},
		&cli.StringFlag{
			Name:  "config, cfg",
			Usage: "load the configuration file at `PATH`",
			Value: "./config.yml",
		},
	}
}

@coilysiren
Copy link
Member Author

@bonedaddy 👍 I added these lines

  • Removed the ability to specify multiple entries in the Command.Name field
    • when updating, replace Name: "a, b, c" with Name: "a", Aliases: []string{"b", "c"}

@bonedaddy
Copy link

@bonedaddy I added these lines

  • Removed the ability to specify multiple entries in the Command.Name field

    • when updating, replace Name: "a, b, c" with Name: "a", Aliases: []string{"b", "c"}

sweet, thank you 🚀

@TomOnTime
Copy link
Contributor

I kept notes as I did a recent conversion. I think these are going to be the typical "first steps" for people (well... people like me!)

  1. Import string changed
  • OLD: import "github.com/urfave/cli"
  • NEW: import "github.com/urfave/cli/v2"
  1. Flag aliases are done differently.

Change Name: "foo, f to "Name: "foo", Aliases: []string{"f"}`

(Steal the examples in
#921 (comment)
...they are better)

  1. Occurrences of []Command have been changed to []*Command

What this means to you:

a. Look for []cli.Command{} and change it to []*cli.Command{}

Example:

  • OLD: var commands = []cli.Command{}
  • NEW: var commands = []*cli.Command{}

Compiler messages you might see:

commands/commands.go:56:30: cannot convert commands (type []cli.Command) to type cli.CommandsByName commands/commands.go:57:15: cannot use commands (type []cli.Command) as type []*cli.Command in assignment

b. If you are building a list of commands, change to pointers

  • OLD: cli.Command{
  • NEW: &cli.Command{

Compiler messages you might see:

./commands.go:32:34: cannot use cli.Command literal (type cli.Command) as type *cli.Command in argument to

  1. cli.Flag is now a list of pointers

What this means to you:

If you make a list of flags, add a & in front of each
item. cli.BoolFlag, cli.StringFlag, etc

  • OLD:
    app.Flags = []cli.Flag{ cli.BoolFlag{

  • NEW:
    app.Flags = []cli.Flag{ &cli.BoolFlag{

Compiler messages you might see:

cli.StringFlag does not implement cli.Flag (Apply method has pointer receiver)

b. appending to a list of commands needs pointers:

  • OLD: commands = append(commands, *c)
  • NEW: commands = append(commands, c)

Compiler messages you might see:

commands/commands.go:28:19: cannot use c (type *cli.Command) as type cli.Command in append

  1. A command's Action: now returns an error
  • OLD: Action: func(c *cli.Context) {
  • NEW: Action: func(c *cli.Context) error {

Compiler messages you might see:

`
./commands.go:35:2: cannot use func literal (type func(*cli.Context)) as type cli.ActionFunc in field value

@coilysiren
Copy link
Member Author

Those are good notes, thanks @TomOnTime

@TomOnTime
Copy link
Contributor

Those are good notes, thanks @TomOnTime

Thanks!

If I may be so bold: I think the typical user of urfave/cli would rather have a few conversion tips now than the perfect document in the future. I would be honored if you published my notes as the starting point; people could send PRs with improvements over time.

It turns out that the conversion process is pretty easy, but I had been avoiding it because I didn't know how hard or difficult it would be. Luckily I had some free time this weekend and decided to guess my way through the compiler errors and see if it would "just work". Other people might not be so willing to "just try it". A nudge in the docs would empower people to give it a try.

@coilysiren
Copy link
Member Author

If I may be so bold: I think the typical user of urfave/cli would rather have a few conversion tips now than the perfect document in the future.

I'll follow up on this sometime soon 👍

@nodefortytwo
Copy link

I'm not sure if this is intentional, a side effect or something I have done wrong but after moving from v1 to v2, flags must come before args ie:

This will work

cli hello --shout rick

This will not

cli hello rick --shout

If this is intentional i couldn't find it documented anywhere and is a major gotcha for migrating.

@TomOnTime
Copy link
Contributor

I took a swing at this. See #1098

@coilysiren
Copy link
Member Author

@nodefortytwo that change isn't specific to V2, it's an issue where the behavior flipped back and forth in V1 too. Here's an example of me changing some V1 code for this problem => #872

@coilysiren
Copy link
Member Author

@nodefortytwo if you have capacity, I would welcome contributors for more test cases here! It's never been 100% clear what the ideal behavior should be, which is why the behavior has shifted over time.

@guo1017138
Copy link

In cli.StringFlag type,
V1 expression is EnvVar: "XXXXX"
V2 expression is EnvVars: []string{"XXXXX"}

@TomOnTime TomOnTime mentioned this issue May 12, 2020
4 tasks
@TomOnTime
Copy link
Contributor

@guo1017138 ... please take a look at #1137

@amacneil
Copy link

amacneil commented Aug 8, 2020

Another change to document - c.GlobalBool(), c.GlobalString() and friends seem to have gone.

@torkelrogstad
Copy link

cli.Context.Parent() is gone, and seems to be replaced by cli.Context.Lineage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/documentation describes a documentation update status/confirmed confirmed to be valid, but work has yet to start
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants