Introduce common utility method for handling map like options

[rhuss]

As we have now multiple options which are of map type we should use a common codebase to guarantee that they are all handled the same

The options I have in mind are (but maybe more):

• --env, --annotations and --label for services
• --filter for triggers
• --traffic for services (see comments below why --traffic is special)

The behaviour should be (using --env as an example):

• --env a1=b1 --env a2=b2 should add both env variables
• --env a1 should set a1 to empty string
• --env a1- should remove the environment variable for update and an error for create
• --env a1=b1 --env a1=b2 should be an error as the same key can not be specified on the same command line

I could imagine to have a function signature like

parseMapOptions(key string, options []string) (map[string]string, error)

and a usage

filters, err := parseMapOptions("filter", f.Filters)

[navidshaikh]

The difference here with --traffic flag is, its explicit; whats given on CLI gets into the spec, no union of existing+given traffic targets. Flag --traffic doesn't support removal sign - too.

[rhuss]

@navidshaikh ok, I see, so let's remove --traffic from the list.

However, there are still enough other use cases which justify a common approach (hopefully ;)

[MIBc]

The env, label and annotations option use a common method mapFromArray to deal with string list.
Is it ok to check same key in mapFromArray?

[daisy-ycguo]

I think this one is related with issue 565 .

I propose to add a new flag type under pkg/kn/commands/flags, named "ArrayList" to support all array flags, and then we can add a method parseMapOptions(key string, options []string) (map[string]string, error) to type ArrayList.

[rhuss]

The env, label and annotations option use a common method mapFromArray to deal with string list.
Is it ok to check same key in mapFromArray?

Wdym with "check same key" ? Actually this method does similar the same but does not handle removal of keys with minus suffix. So maybe this method can replaced or extended by the algorithm described above.

[rhuss]

I think this one is related with issue 565 .

Yes

I propose to add a new flag type under pkg/kn/commands/flags, named "ArrayList" to support all array flags, and then we can add a method parseMapOptions(key string, options []string) (map[string]string, error) to type ArrayList.

Sorry, I don't get why adding of new type would make things easier than a simple function. Would be cool if you give an example.

[daisy-ycguo]

Sorry, I don't get why adding of new type would make things easier than a simple function. Would be cool if you give an example.

My mistake. no need to add a new type.

[MIBc]

Can filter be set like --fliter type= ? @rhuss
I write a trigger yaml file and the filter section like this:

filter:
    attributes:
      type: ""

It doesn't raise error. Does that mean filter type value can be empty?