search

kubernetes-sigs / legacyflag

Provides a pflag wrapper that addresses pain-points of backwards-compatible ComponentConfig migration.
Apache License 2.0
2 stars 8 forks source link

Implemented VarValue.Value method #4

Closed alejandrox1 closed 5 years ago

alejandrox1 commented 5 years ago

This method will enable to user to access the value for the registered Flag in the VarValue FlagSet.

Signed-off-by: alejandrox1 alarcj137@gmail.com

k8s-ci-robot commented 5 years ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alejandrox1 To complete the pull request process, please assign mtaufen You can assign the PR to them by writing /assign @mtaufen in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/kubernetes-sigs/legacyflag/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
alejandrox1 commented 5 years ago

Hi all,wanted to propose this one. So currently people can create their custom flags and register these as 

flag.Var(SOMEVal{Val: value})

Within legacyflag, this would create a VarValue. The problem is that there is no way of getting the value of the registered flag from the VarValue struct afterwards which is an essential part of the override pattern that was proposed in https://github.com/kubernetes/kubernetes/pull/73494/files#diff-3c02581ed8bf668707aaade4978f7281R55 .

I have an example of how this could work in https://github.com/kubernetes/kubernetes/pull/79916/files#diff-cf93bbed37202b7d58b5841f07ddd89fR208

I would love any feedback, comments, criticisms you all may have :smiley:

sttts commented 5 years ago

/assign @mtaufen

mtaufen commented 5 years ago

I want to avoid interface{} so we maintain type-safety. This was the reason for Apply just taking a func() as an argument - it forces you to use a closure to capture the value, which is type-safe.

I think you're right that it's not entirely clear how to use Apply in this case. The following example is roughly what I was thinking:

func AddFlags() (apply func(c *config.Config)) {
  var scratch *SomeVal
  val := fs.Var(scratch, "someval", "")
  afn := func(c *config.Config)  {
    val.Apply(func(){
      c.someval = scratch
    })
  }
  return afn
}
alejandrox1 commented 5 years ago

sorry for the delay on this but thank you for your comments! This makes sense :+1: /close

k8s-ci-robot commented 5 years ago

@alejandrox1: Closed this PR.

In response to [this](https://github.com/kubernetes-sigs/legacyflag/pull/4#issuecomment-532648684): >sorry for the delay on this but thank you for your comments! >This makes sense :+1: >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.