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...

[stevekuznetsov]

I'm not sure I am the biggest fan of how this looks to use as a consumer and the loss of type safety. I wonder how complex the Go program would be to auto-generate a typed tree struct given a type?

[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: #9757 (comment)
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?

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[matthyx]

@BenTheElder I should present this work to the sig-testing meeting one day... to check if it still makes sense to modify the configuration format.
/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[cblecker]

/remove-lifecycle stale
/cc @MadhavJivrajani

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-ci-robot]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle rotten
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[matthyx]

/reopen
will rebase and merge once we move prow repo

[k8s-ci-robot]

@matthyx: Reopened this PR.

In response to this:

/reopen
will rebase and merge once we move prow repo

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: matthyx
Once this PR has been reviewed and has the lgtm label, please ask for approval from cjwagner. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• prow/plugins/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

@matthyx: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-test-infra-lint	0fb05cb	link	/test pull-test-infra-lint
pull-test-infra-verify-config	0fb05cb	link	/test pull-test-infra-verify-config
pull-test-infra-prow-checkconfig	f45c923	link	/test pull-test-infra-prow-checkconfig
pull-test-infra-misc-image-build-test	b7bf1a2	link	true	/test pull-test-infra-misc-image-build-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[jihoon-seo]

Hello @matthyx, the Prow source code has been moved to https://github.com/kubernetes-sigs/prow on April 9, 2024.
Please consider migrate this PR (by opening a new one in there).
Thanks!

[matthyx]

Yes thanks for the reminder I'll do this soon

[matthyx]

kubernetes-sigs/prow#147