Implement ConfigTree in approve plugin

[matthyx]

Fixes #9757 @cjwagner I couldn't make this work without encapsulating the config inside a config section... if you have any idea on how to make that work with json.RawMessage pls advise.

[matthyx]

Hmm, looks like I have to tweak func checkUnknownFields for the RawMessage parts...

[matthyx]

I am not sure it's possible to generate types at runtime in Go... I did try several approaches, but that one had the benefit of using generic methods for the get repo/org/branch and we get the default behavior of maps for merging.

[stevekuznetsov]

Sorry, not at runtime but at compile time -- we can check in the generated code. Something like this: https://github.com/taylorchu/generic

The runtime/ast library is wonderful

[matthyx]

But this is a bit against what @cjwagner was saying here: https://github.com/kubernetes/test-infra/issues/9757#issuecomment-428647392 Should we ask him?

[cjwagner]

Instead of having the leaf config struct type be json.RawMessage I was imagining we could use an interface:

type LeafConfig interface {
  MarshalJSON() ([]byte, error)
  UnmarshalJSON([]byte) error
  Apply(parent LeafConfig) (LeafConfig, error)
}

Concrete implementations of LeafConfig would have to use a type cast in the Apply function to convert parent to the concrete type and plugins would similarly have to cast to the concrete type when using the config, but that doesn't seem like a big deal. This pattern will also allow for flexible LeafConfig merging behavior so that we can do more than just have the child config overwrite the top level keys of the parent config (e.g. append slices together, merge maps, etc.).

I'd also be ok with Steve's suggestion, but I'd prefer to use the features go provides instead of code generation if possible.

[matthyx]

Thanks, I will try my best :)

[stevekuznetsov]

I'm pretty sure the ast package et al are provided by the language specifically to allow people to do what I mentioned. Does the interface you expected still require the FillConfig style call when we want to use the fields?

[cjwagner]

I'm pretty sure the ast package et al are provided by the language specifically to allow people to do what I mentioned.

I'm not familiar with that package and https://golang.org/pkg/go/ast/#pkg-examples doesn't really make it clear how we would use the package for these purposes. How would you use the ast package to define and merge the leaf configs?

Does the interface you expected still require the FillConfig style call when we want to use the fields?

No. I don't think that is even necessary with the current implementation. We can and should be applying the parent configs to child configs when the config is first loaded. We probably still want a getter func to retrieve the most specific config that is available given an org, repo, and branch (like we have for branch protection).

[stevekuznetsov]

You generate code by building the AST then printing it -- that's what is used for all of the generated clients, conversions, deep-copies for k8s, etc.

If we have an interface-based approach that doesn't need the FillConfig step it seems reasonable, but I would like to see typed accessors and good validation on load as well if possible, so that downstream code doesn't touch interface{}

[cjwagner]

AST sounds cool and would likely provide the cleanest API, but I think the interface approach will be nearly as clean, will probably be simpler, and will allow for more flexible leaf config merging so thats what I'm leaning towards right now. I'm totally ok with either approach though. The tradeoffs seem like they balance out.

[stevekuznetsov]

Agreed -- the implementation in the PR today resolves the values at runtime and offloads that to the caller which I think is not the most clean implementation, but if we can get typed accessors and validation on config load I think the interface based approach would be perfect.

[matthyx]

@cjwagner one question though... what type should I put in config.go for Approve? This is the same issue I had faced before falling back to rawMessage.

If I put ConfigTree as it is now on my branch, I don't see how the unmarshaller will understand it contains some Approve structs, even if Approve implement LeafConfig interface?

It seems (I might be wrong) that somehow I have to declare all the chain ApproveConfigTree, ApproveOrg, ... In that case I much prefer the code generation path :-/

[cjwagner]

If I put ConfigTree as it is now on my branch, I don't see how the unmarshaller will understand it contains some Approve structs, even if Approve implement LeafConfig interface?

Hmmm, we can't unmarshal into a nil interface, but we should be able to unmarshal into a populated interface variable since it implements Unmarshaller. I was thinking we could just populate the interfaces with the concrete type before unmarshalling to them, but that can't be easily done for the LeafConfigs that are stored as map values because we don't know which keys to specify concrete values for until after we have parsed. I think our options are:

1. Parse the maps without the leaf configs first to determine the extant keys, then specify concrete LeafConfig structs for each of the identified keys and reparse with the leaf configs included.
2. Avoid using interfaces and define the full chain of structs like you described.
3. Resort to code generation.

[stevekuznetsov]

Generation is what makes k8s apimachinery work and honestly I think it's a bit scary up-front but the output is really simple -- reviewable Go code, etc. I think it might be our best bet here.

[matthyx]

I will experiment, but it'll be later as I'm handling my KEP for 1.15 in priority...

[matthyx]

I was thinking of trying with https://github.com/google/gvisor/blob/master/tools/go_generics/generics.go any thoughts?

[stevekuznetsov]

That seems like a low cost of entry way to do this, let's see how it works!

[matthyx]

I now need to fix update-config to not mess up the go_generics specific config...

[matthyx]

@cjwagner I think I need help to adapt hack/* scripts to code generation :'( I also need your thoughts on non-anonymous fields in the tree, required by the generics lib (see comment).

[cjwagner]

@cjwagner I think I need help to adapt hack/* scripts to code generation :'(

Ok, what exactly are you trying to do? It looks like you are having bazel handle code generation at compile time so I'm not sure we need to check in generated code (and therefore don't need to change the code gen scripts in hack/).

I also need your thoughts on non-anonymous fields in the tree, required by the generics lib (see comment).

I don't really understand the problem or question here. What isn't working and in what way is it not working?

[matthyx]

I also need your thoughts on non-anonymous fields in the tree, required by the generics lib (see comment).

Regarding the format of the configuration yaml, here is what you wanted:

issue_required: false
require_self_approval: false
lgtm_acts_as_approve: false
ignore_review_state: true
orgs:
  bazelbuild:
    ignore_review_state: false
  kubernetes:
    lgtm_acts_as_approve: true
    repos:
      kops:
        lgtm_acts_as_approve: false

Here is what we need to have because of this limitation:

config:
  issue_required: false
  require_self_approval: false
  lgtm_acts_as_approve: false
  ignore_review_state: true
orgs:
  bazelbuild:
    config:
      ignore_review_state: false
  kubernetes:
    config:
      lgtm_acts_as_approve: true
    repos:
      kops:
        config:
          lgtm_acts_as_approve: false

[matthyx]

Ok, what exactly are you trying to do? It looks like you are having bazel handle code generation at compile time so I'm not sure we need to check in generated code (and therefore don't need to change the code gen scripts in hack/).

I am trying to make tests pass... looks like the 3 failures are related to checkconfig but I have no idea how to fix it :-(

[matthyx]

@cjwagner PTAL?

[matthyx]

@stevekuznetsov @cjwagner ready to review

[matthyx]

/assign @cjwagner

[cjwagner]

With respect to https://github.com/kubernetes/test-infra/pull/12440#issuecomment-500702078: Having to include the extra config: level makes the YAML a lot harder to read. I'd really like to avoid that if possible. I can't think of a reason why it wouldn't be possible for go_generics to handle embedded types. Could you see if that would be a hard change to upstream or just open an issue so we can get some feedback from the code owners about this?

[matthyx]

Could you see if that would be a hard change to upstream or just open an issue so we can get some feedback from the code owners about this?

https://github.com/google/gvisor/pull/504

[matthyx]

@cjwagner I have the merge in gvisor for go_generics and implemented the tests and everything.

Regarding the legacy config, please let me know what you think of the second commit... it seems from what I have tried that I need to use a json.RawMessage and try to unmarshal in both formats.

I still have to fix some tests, but at least I wanna get your opinion on what I have so far... thx!

[fejta-bot]

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

[matthyx]

/remove-lifecycle stale