Test-infra: checkconfig returns error for valid config

[clarketm]

What happened:
checkconfig returns an error for a valid config. More specifically, checkUnknownFields(..) returns unknown fields that are, in fact, known.

https://github.com/kubernetes/test-infra/blob/c730fab62edb4442547269b17e314b6dd861e925/prow/cmd/checkconfig/main.go#L402-L431

What you expected to happen:
A valid, functional configuration should be considered valid by checkconfig.

How to reproduce it (as minimally and precisely as possible):

1. Add a legacy slack_reporter field to your Prow configuration.
2. Run checkconfig with --strict and/or --warnings=unknown-fields warning enabled.

Please provide links to example occurrences, if any:
example

[clarketm]

/help

[stevekuznetsov]

Copying over from that thread ...

That is totally bizarre. I think we should reconsider the bespoke reflection-based code we have in there to check for bad fields. I don't think that it is worth the effort to maintain it over strict YAML loading as allowed by the library. The only reason I remember being given for the code as-is, was that the library only told you about one unknown field at once. At least it is more robust, I would expect. This code keeps biting us and I don't think it's given us much value...

@cjwagner @fejta

[afirth]

warns about org: too (peribolos)

docker run -it -v /home/afirth/git/github-automation:/tmp gcr.io/k8s-prow/checkconfig \
--plugin-config=/tmp/plugins.yaml \
--config-path=/tmp/config.yaml
{"component":"checkconfig","file":"prow/cmd/checkconfig/main.go:76","func":"main.reportWarning","level":"warning","msg":"unknown fields present in /tmp/config.yaml: orgs","time":"2020-01-15T14:20:29Z"}

[stevekuznetsov]

On Slack consensus was reached to remove the reflection-based validation and just to use the strict YAML loading -- @afirth do you want to take a stab at that?

[matthyx]

/cc

[afirth]

On Slack consensus was reached to remove the reflection-based validation and just to use the strict YAML loading -- @afirth do you want to take a stab at that?

Sure. Do we need a flag to leave the old unstrict behaviour? Looks pretty straightforward to rip out all the unknown field stuff, just not sure how others are using this

[afirth]

/assign @afirth

[matthyx]

/remove-help