Flag web-config-apps as string instead of string slice

[refs]

I rather parse a comma separated string values into a []string than fight StringSliceFlag. Documentation has been added.

For more information have a look at:

urfave/cli#392 (part of urfave/cli v2)
urfave/cli#160 (closed in v2, we're using v1)

[butonic]

@refs where do you actually cut the apps into parts?

[refs]

@butonic in the ocis PhoenixCommand. Here not just yet, as it is not used anywhere yet in this project.

[refs]

this is being followed up by a PR on ocis where I actually make use of it. I'm however adding a helper to phoenix flagset to capture this behavior within this package, as it's not ocis concern to do it. this functionality was moved to owncloud/ocis-pkg

[tboerger]

Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- pkg/command/server.go  1
         

See the complete overview on Codacy

[tboerger]

That sounds odd, the default usage of string slice flags seems much more common to provide the flag multiple times instead of the comma separated list.

[refs]

That sounds odd, the default usage of string slice flags seems much more common to provide the flag multiple times instead of the comma separated list.

🤷‍♂ I find repeating the web-config-apps more arduous than just a comma separated set of values. WDYT? Is this common?

[tboerger]

which problem do you want to resolve that way? Only the missing Destination flag? I personally resolve that issue (until it gets fixed upstream) be assigning the string slice to the config struct within a Before handler.

[refs]

StringSlice flags defaults are not overridden, as urfave/cli#160 specifies.

I cracked a small example:

package main

import (
	"fmt"
	"log"
	"os"

	"github.com/urfave/cli"
)

func main() {
	app := cli.NewApp()
	app.Name = "greet"
	app.Usage = "fight the loneliness!"
	app.Action = func(c *cli.Context) error {
		value := c.StringSlice("list")
		fmt.Println(value)

		return nil
	}
	app.Flags = []cli.Flag{
		cli.StringSliceFlag{
			Name:  "list",
			Value: &cli.StringSlice{"1", "2", "3"},
		},
	}

	err := app.Run(os.Args)
	if err != nil {
		log.Fatal(err)
	}
}

./urfave --list 4 --list 5 --list 6 // [1 2 3 4 5 6]

This is totally broken IMO, it doesn't override default values!

[tboerger]

We could also assign the default if the slice is empty within Before, submit a PR to micro cli, and remove the Before workaround when we got it merged upstream. That way everybody using micro would benefit ;)

[tboerger]

And that way AFAIK both fractions can be happy because if I remember correctly comma separated strings are transformed into the string slice automatically while others can use the expected behavior --list 4 --list 5 --list 6 :D

[refs]

As messed around this morning this would mean a fix upstream (upstream being urfavecli) as it still presents this bug: urfave/cli#549 Until I (or someone else) contributes to urfavecli fixing it, then adding the fix into micro/cli, it might be a viable option to have our fix merged on our end, move on, and tag this to deal with it at its time.

[tboerger]

Great, merging without talking about my complains...

[butonic]

I was told this was discussed in the office and ok to merge. 🤷‍♂️

I guess the question is how we keep track of the upstream bug? We are not reusing projects, so adding the external bug to the current sprint is possible but we would risk loosing track of it.

Edit: added as https://github.com/orgs/owncloud/projects/142#card-31751793

[refs]

It was my call; assumed things were clear when we discussed them at the office. I'll follow this up with a set of PR's on urfave / micro / owncloud. Hopefully it gets updated upstream.