I'm not happy with the name.

Perhaps CopyFlags(from *pflag.FlagSet, to *LoggingConfiguration)?

do you want to make this part of the log api?

should not handle this directly in kube-proxy since it already has a precedent overriding a flag #119864 (comment)

This way you can come with a more universal solution in 1.28

I think the final solution may be something like this. Doing this entirely in kube-proxy as a hot fix now is easier than trying to get it implemented properly here.

OTOH, this is just a Go API. It can be changed at any time without breaking the API of components using it, only compilation of the component may break...

your call, I just wanted to give you more options, with kube-proxy I think that is fair to go with the override in 1.28

Perhaps CopyFlags(from *pflag.FlagSet, to *LoggingConfiguration)?

OverrideLogsFromFlags(from *pflag.FlagSet, to *LoggingConfiguration) ?

Perhaps CopyFlags(from *pflag.FlagSet, to *LoggingConfiguration)?

OverrideLogsFromFlags(from *pflag.FlagSet, to *LoggingConfiguration) ?

For --vmodule, the flag doesn't override: instead, the entries from the config and the command line get merged, with command line entries at the beginning where they have priority. I think that's the right semantic.

But therefore "override" isn't quite appropriate, so I'm going with "copy".

Okay, got it.

Moving this after the 1.28 release?

yes, it has to be later

Let's to it in 1.29.

Can it be moved now? I can work on this.

Yes, and please do.

Some points to consider:

• Can the same be done for apps using just plain Go flags? The "flag was set" field only exists for pflag. A potential solution might be to wrap the values in a flag.Value implementation which tracks whether Set was called.
• Examples and docs...
  • The original plan was to have a JSON or YAML config that gets decoded into the LoggingConfiguration instance after parsing, in which case this new function isn't needed. It needs to be checked that this works and be documented.
  • For configs where that doesn't work (i.e. anything based on registered types via apimachinery like this config here), the new functions need to be documented.

A potential solution might be to wrap the values in a flag.Value implementation which tracks whether Set was called.

I found it hard to check if a go flag was set, I tried to check it by using the following codes, but it didn't work.

func isFlagSet(name string) bool {
    found := false
    flag.Visit(func(f *flag.Flag) {
        if f.Name == name {
            found = true
        }
    })
    return found
}

Could you give me an example?

and we may need a new function called copyLogsFromGoFlags for this, It may not be for Kubernetes users.

A wrapper as in this example is one possible solution:

package main

import (
	"flag"
	"fmt"
)

type flagWrapper struct {
	flag.Value
	isSet bool
}

func (f *flagWrapper) Set(value string) error {
	f.isSet = true
	return f.Value.Set(value)
}

var _ flag.Value = &flagWrapper{}

func main() {
	var fs flag.FlagSet
	fs.Int("some-int", 42, "the answer")
	arg := fs.Lookup("some-int")
	wrapper := &flagWrapper{Value: arg.Value}
	flag.CommandLine.Var(wrapper, arg.Name, arg.Usage)
	flag.Parse()
	fmt.Printf("some-int set = %v\n", wrapper.isSet)
}

The downside is that -h doesn't know about the actual type of the parameter:

$ go run . -h
Usage of /tmp/go-build544671316/b001/exe/testflag:
  -some-int value
    	the answer

Without the wrapper, int would be shown instead of value. That's because flag looks at the type of a value and has special support for builtin value types.

Thanks for the example.

The downside is that -h doesn't know about the actual type of the parameter

Can we explain in the usage what the actual type of the parameter is?

Yes. Showing "value" isn't a showstopper.

this scopes the change to the Logging configuration that makes this change contained

+1

should we make the Flags part of the Options struct instead of plumbing it down?
I see kube-scheduler has them there too, it feels like is cleaner

Perhaps, but that would be a larger change that I don't want to do this close to a release.

sgtm

I can't really evaluate this logic, I trust other reviewers

If I understand it correctly it overrides everything in logging present in flags, except the to.Vmodule that gets appended